Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-9350

Avoid creating temprorary strings in Block.toString() and getBlockName()

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.7.1
    • Fix Version/s: 2.9.0, 3.0.0-alpha1
    • Component/s: performance
    • Labels:
      None
    • Target Version/s:

      Description

      Minor change to use StringBuilders directly to avoid creating temporary strings of Long and Block name when doing toString on a Block.

      1. HDFS-9350.001.patch
        1 kB
        Staffan Friberg

        Issue Links

          Activity

          Hide
          daryn Daryn Sharp added a comment -

          I know SB should be used to append inside a loop, but doesn't the jvm use a SB internally for simple plus operations? If yes, explicit use of a SB only seems to decrease readability.

          Show
          daryn Daryn Sharp added a comment - I know SB should be used to append inside a loop, but doesn't the jvm use a SB internally for simple plus operations? If yes, explicit use of a SB only seems to decrease readability.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 8s docker + precommit patch detected.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          -1 mvninstall 1m 38s root in trunk failed.
          +1 compile 0m 26s trunk passed with JDK v1.8.0_60
          +1 compile 0m 23s trunk passed with JDK v1.7.0_79
          +1 checkstyle 0m 10s trunk passed
          +1 mvneclipse 0m 10s trunk passed
          +1 findbugs 1m 48s trunk passed
          +1 javadoc 0m 18s trunk passed with JDK v1.8.0_60
          +1 javadoc 0m 23s trunk passed with JDK v1.7.0_79
          +1 mvninstall 0m 28s the patch passed
          +1 compile 0m 23s the patch passed with JDK v1.8.0_60
          +1 javac 0m 23s the patch passed
          +1 compile 0m 24s the patch passed with JDK v1.7.0_79
          +1 javac 0m 24s the patch passed
          +1 checkstyle 0m 10s the patch passed
          +1 mvneclipse 0m 11s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          +1 findbugs 1m 55s the patch passed
          +1 javadoc 0m 18s the patch passed with JDK v1.8.0_60
          +1 javadoc 0m 22s the patch passed with JDK v1.7.0_79
          +1 unit 0m 50s hadoop-hdfs-client in the patch passed with JDK v1.8.0_60.
          +1 unit 0m 51s hadoop-hdfs-client in the patch passed with JDK v1.7.0_79.
          +1 asflicense 0m 43s Patch does not generate ASF License warnings.
          14m 30s



          Subsystem Report/Notes
          Docker Client=1.7.1 Server=1.7.1 Image:test-patch-base-hadoop-date2015-11-02
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12769869/HDFS-9350.001.patch
          JIRA Issue HDFS-9350
          Optional Tests asflicense javac javadoc mvninstall unit findbugs checkstyle compile
          uname Linux 80bb0c86f4c9 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /home/jenkins/jenkins-slave/workspace/PreCommit-HDFS-Build/patchprocess/apache-yetus-e77b1ce/precommit/personality/hadoop.sh
          git revision trunk / 90e1405
          Default Java 1.7.0_79
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_60 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_79
          mvninstall https://builds.apache.org/job/PreCommit-HDFS-Build/13333/artifact/patchprocess/branch-mvninstall-root.txt
          findbugs v3.0.0
          JDK v1.7.0_79 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/13333/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs-client U: hadoop-hdfs-project/hadoop-hdfs-client
          Max memory used 65MB
          Powered by Apache Yetus http://yetus.apache.org
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/13333/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 8s docker + precommit patch detected. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 mvninstall 1m 38s root in trunk failed. +1 compile 0m 26s trunk passed with JDK v1.8.0_60 +1 compile 0m 23s trunk passed with JDK v1.7.0_79 +1 checkstyle 0m 10s trunk passed +1 mvneclipse 0m 10s trunk passed +1 findbugs 1m 48s trunk passed +1 javadoc 0m 18s trunk passed with JDK v1.8.0_60 +1 javadoc 0m 23s trunk passed with JDK v1.7.0_79 +1 mvninstall 0m 28s the patch passed +1 compile 0m 23s the patch passed with JDK v1.8.0_60 +1 javac 0m 23s the patch passed +1 compile 0m 24s the patch passed with JDK v1.7.0_79 +1 javac 0m 24s the patch passed +1 checkstyle 0m 10s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 1m 55s the patch passed +1 javadoc 0m 18s the patch passed with JDK v1.8.0_60 +1 javadoc 0m 22s the patch passed with JDK v1.7.0_79 +1 unit 0m 50s hadoop-hdfs-client in the patch passed with JDK v1.8.0_60. +1 unit 0m 51s hadoop-hdfs-client in the patch passed with JDK v1.7.0_79. +1 asflicense 0m 43s Patch does not generate ASF License warnings. 14m 30s Subsystem Report/Notes Docker Client=1.7.1 Server=1.7.1 Image:test-patch-base-hadoop-date2015-11-02 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12769869/HDFS-9350.001.patch JIRA Issue HDFS-9350 Optional Tests asflicense javac javadoc mvninstall unit findbugs checkstyle compile uname Linux 80bb0c86f4c9 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /home/jenkins/jenkins-slave/workspace/PreCommit-HDFS-Build/patchprocess/apache-yetus-e77b1ce/precommit/personality/hadoop.sh git revision trunk / 90e1405 Default Java 1.7.0_79 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_60 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_79 mvninstall https://builds.apache.org/job/PreCommit-HDFS-Build/13333/artifact/patchprocess/branch-mvninstall-root.txt findbugs v3.0.0 JDK v1.7.0_79 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/13333/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs-client U: hadoop-hdfs-project/hadoop-hdfs-client Max memory used 65MB Powered by Apache Yetus http://yetus.apache.org Console output https://builds.apache.org/job/PreCommit-HDFS-Build/13333/console This message was automatically generated.
          Hide
          sfriberg Staffan Friberg added a comment -

          It does, but the problem is that the call to Long.toString will actually allocate a new String to be appended, and the call to getBlockname sometimes (unless correctly inlined will do the same). So you don't get a single append chain with SB in this case.

          Show
          sfriberg Staffan Friberg added a comment - It does, but the problem is that the call to Long.toString will actually allocate a new String to be appended, and the call to getBlockname sometimes (unless correctly inlined will do the same). So you don't get a single append chain with SB in this case.
          Hide
          cmccabe Colin P. McCabe added a comment -

          That makes sense.

          +1. Thanks, Staffan Friberg.

          Show
          cmccabe Colin P. McCabe added a comment - That makes sense. +1. Thanks, Staffan Friberg .
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #8998 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8998/)
          HDFS-9350. Avoid creating temprorary strings in Block.toString() and (cmccabe: rev e63388fdf22b5fd20ca00f9fad9f40656f117d95)

          • hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/Block.java
          • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #8998 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8998/ ) HDFS-9350 . Avoid creating temprorary strings in Block.toString() and (cmccabe: rev e63388fdf22b5fd20ca00f9fad9f40656f117d95) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/Block.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Hide
          cnauroth Chris Nauroth added a comment -

          I think this patch should be reverted, because it broke the contract of the static Block#toString method. Here is the code before the patch:

            /**
             * A helper method to output the string representation of the Block portion of
             * a derived class' instance.
             *
             * @param b the target object
             * @return the string representation of the block
             */
            public static String toString(final Block b) {
              return getBlockName() + "_" + getGenerationStamp();
            }
          

          Here is the code after the patch:

            /**
             * A helper method to output the string representation of the Block portion of
             * a derived class' instance.
             *
             * @param b the target object
             * @return the string representation of the block
             */
            public static String toString(final Block b) {
              StringBuilder sb = new StringBuilder();
              b.appendStringTo(sb);
              return sb.toString();
            }
          

          Notice that the JavaDocs say this method is supposed to provide just the basic Block data, even in cases where the parameter is a subclass that may have overridden appendStringTo. Having a widening cast to Block in the method signature's argument does not influence the outcome of runtime dynamic dispatch. One specific problem would be ReplicaUnderConstruction, which overrides appendStringTo, so passing an instance of that will not in fact give the caller the basic Block data.

          Thoughts? Cc'ing Wei-Chiu Chuang, who I think is the original author of the static Block#toString method. He can help confirm the intent of the method.

          Show
          cnauroth Chris Nauroth added a comment - I think this patch should be reverted, because it broke the contract of the static Block#toString method. Here is the code before the patch: /** * A helper method to output the string representation of the Block portion of * a derived class' instance. * * @param b the target object * @ return the string representation of the block */ public static String toString( final Block b) { return getBlockName() + "_" + getGenerationStamp(); } Here is the code after the patch: /** * A helper method to output the string representation of the Block portion of * a derived class' instance. * * @param b the target object * @ return the string representation of the block */ public static String toString( final Block b) { StringBuilder sb = new StringBuilder(); b.appendStringTo(sb); return sb.toString(); } Notice that the JavaDocs say this method is supposed to provide just the basic Block data, even in cases where the parameter is a subclass that may have overridden appendStringTo . Having a widening cast to Block in the method signature's argument does not influence the outcome of runtime dynamic dispatch. One specific problem would be ReplicaUnderConstruction , which overrides appendStringTo , so passing an instance of that will not in fact give the caller the basic Block data. Thoughts? Cc'ing Wei-Chiu Chuang , who I think is the original author of the static Block#toString method. He can help confirm the intent of the method.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Chris Nauroth, you're absolutely right about it. In HDFS-7284, the original intent was to print just the Block portion, not the subclass. We added the static method because typecasting doesn't change runtime binding.

          If the goal is to avoid creating temp strings, I think we could create a static appendStringTo() method, and call the static method instead.

          Show
          jojochuang Wei-Chiu Chuang added a comment - Chris Nauroth , you're absolutely right about it. In HDFS-7284 , the original intent was to print just the Block portion, not the subclass. We added the static method because typecasting doesn't change runtime binding. If the goal is to avoid creating temp strings, I think we could create a static appendStringTo() method, and call the static method instead.
          Hide
          cmccabe Colin P. McCabe added a comment -

          Good catch, Chris Nauroth. I agree that we should restrict this method to just printing data from the base class. However, Staffan's goal of reducing the number of memory allocations is nice. How about this change?

          diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/Block.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/Block.java
          index 23bfa95..4128ece 100644
          --- a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/Block.java
          +++ b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/Block.java
          @@ -162,7 +162,9 @@ public void setGenerationStamp(long stamp) {
              */
             public static String toString(final Block b) {
               StringBuilder sb = new StringBuilder();
          -    b.appendStringTo(sb);
          +    sb.append(BLOCK_FILE_PREFIX).
          +       append(b.blockId).append("_").
          +       append(b.generationStamp);
               return sb.toString();
             }
          
          Show
          cmccabe Colin P. McCabe added a comment - Good catch, Chris Nauroth . I agree that we should restrict this method to just printing data from the base class. However, Staffan's goal of reducing the number of memory allocations is nice. How about this change? diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/Block.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/Block.java index 23bfa95..4128ece 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/Block.java +++ b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/Block.java @@ -162,7 +162,9 @@ public void setGenerationStamp( long stamp) { */ public static String toString( final Block b) { StringBuilder sb = new StringBuilder(); - b.appendStringTo(sb); + sb.append(BLOCK_FILE_PREFIX). + append(b.blockId).append( "_" ). + append(b.generationStamp); return sb.toString(); }
          Hide
          cnauroth Chris Nauroth added a comment -

          Colin P. McCabe, that's perfect. +1.

          Show
          cnauroth Chris Nauroth added a comment - Colin P. McCabe , that's perfect. +1.
          Hide
          cmccabe Colin P. McCabe added a comment -

          Thanks, Chris Nauroth. I uploaded the patch to HDFS-9947. I would have done it earlier, but JIRA was a bit unresponsive.

          Show
          cmccabe Colin P. McCabe added a comment - Thanks, Chris Nauroth . I uploaded the patch to HDFS-9947 . I would have done it earlier, but JIRA was a bit unresponsive.

            People

            • Assignee:
              sfriberg Staffan Friberg
              Reporter:
              sfriberg Staffan Friberg
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development