Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.2
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Bytes.toInt shows up quite often in a profiler run.
      It turns out that one source is HFileReaderV2$ScannerV2.getKeyValue().

      Notice that we call the KeyValue(byte[], int) constructor, which forces the constructor to determine its size by reading some of the header information and calculate the size. In this case, however, we already know the size (from the call to readKeyValueLen), so we could just use that.

      In the extreme case of 10000's of columns this noticeably reduces CPU.

      1. 6621-0.96-v4.txt
        2 kB
        Lars Hofhansl
      2. 6621-0.96-v3.txt
        3 kB
        Lars Hofhansl
      3. 6621-0.96-v2.txt
        2 kB
        Lars Hofhansl
      4. 6621-0.96.txt
        0.7 kB
        Lars Hofhansl

        Activity

        Hide
        stack stack added a comment -

        Fix up after bulk move overwrote some 0.94.2 fix versions w/ 0.95.0 (Noticed by Lars Hofhansl)

        Show
        stack stack added a comment - Fix up after bulk move overwrote some 0.94.2 fix versions w/ 0.95.0 (Noticed by Lars Hofhansl)
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-0.94-security-on-Hadoop-23 #7 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/7/)
        HBASE-6621 Reduce calls to Bytes.toInt (Revision 1375665)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/KeyValue.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java
        Show
        hudson Hudson added a comment - Integrated in HBase-0.94-security-on-Hadoop-23 #7 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/7/ ) HBASE-6621 Reduce calls to Bytes.toInt (Revision 1375665) Result = FAILURE larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/KeyValue.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-0.94-security #48 (See https://builds.apache.org/job/HBase-0.94-security/48/)
        HBASE-6621 Reduce calls to Bytes.toInt (Revision 1375665)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/KeyValue.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java
        Show
        hudson Hudson added a comment - Integrated in HBase-0.94-security #48 (See https://builds.apache.org/job/HBase-0.94-security/48/ ) HBASE-6621 Reduce calls to Bytes.toInt (Revision 1375665) Result = FAILURE larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/KeyValue.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #141 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/141/)
        HBASE-6621 Reduce calls to Bytes.toInt (Revision 1375663)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValue.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java
        Show
        hudson Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #141 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/141/ ) HBASE-6621 Reduce calls to Bytes.toInt (Revision 1375663) Result = FAILURE larsh : Files : /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValue.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Here's another observation. In ScanQueryMatcher.match we have this:

            byte [] bytes = kv.getBuffer();
            int offset = kv.getOffset();
            int initialOffset = offset;
        
            int keyLength = Bytes.toInt(bytes, offset, Bytes.SIZEOF_INT);
        

        At this point the passed kv already has its keyLength cached. So we can use

        int keyLength = kv.getKeyLength();

        instead a save a few more cycles.
        This has measurable effect with many columns (~3%).

        A simple 1-line change. Any opposition doing this here as well, or should I open a new issue.

        Show
        lhofhansl Lars Hofhansl added a comment - Here's another observation. In ScanQueryMatcher.match we have this: byte [] bytes = kv.getBuffer(); int offset = kv.getOffset(); int initialOffset = offset; int keyLength = Bytes.toInt(bytes, offset, Bytes.SIZEOF_INT); At this point the passed kv already has its keyLength cached. So we can use int keyLength = kv.getKeyLength(); instead a save a few more cycles. This has measurable effect with many columns (~3%). A simple 1-line change. Any opposition doing this here as well, or should I open a new issue.
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-TRUNK #3250 (See https://builds.apache.org/job/HBase-TRUNK/3250/)
        HBASE-6621 Reduce calls to Bytes.toInt (Revision 1375663)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValue.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java
        Show
        hudson Hudson added a comment - Integrated in HBase-TRUNK #3250 (See https://builds.apache.org/job/HBase-TRUNK/3250/ ) HBASE-6621 Reduce calls to Bytes.toInt (Revision 1375663) Result = FAILURE larsh : Files : /hbase/trunk/hbase-common/src/main/java/org/apache/hadoop/hbase/KeyValue.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-0.94 #411 (See https://builds.apache.org/job/HBase-0.94/411/)
        HBASE-6621 Reduce calls to Bytes.toInt (Revision 1375665)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/KeyValue.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java
        Show
        hudson Hudson added a comment - Integrated in HBase-0.94 #411 (See https://builds.apache.org/job/HBase-0.94/411/ ) HBASE-6621 Reduce calls to Bytes.toInt (Revision 1375665) Result = FAILURE larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/KeyValue.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderV2.java
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Thanks for the reviews.

        Show
        lhofhansl Lars Hofhansl added a comment - Thanks for the reviews.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Committed to 0.94 and 0.96.

        Show
        lhofhansl Lars Hofhansl added a comment - Committed to 0.94 and 0.96.
        Hide
        stack stack added a comment -

        I +1'd v2 Lars. v4 looks good to me.

        Show
        stack stack added a comment - I +1'd v2 Lars. v4 looks good to me.
        Hide
        tlipcon Todd Lipcon added a comment -

        Oops, yea, I missed the fact that the caching was removed again in the later patches. The change makes sense to me.

        Show
        tlipcon Todd Lipcon added a comment - Oops, yea, I missed the fact that the caching was removed again in the later patches. The change makes sense to me.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Using a qualifier filter that just filters each qualifier (to eliminate variance from the network and the driver client), I see 7.30ms vs 7.65ms per, a ~4% improvement.

        This was a repeated get of a row with 200.000 columns, all of which are filtered by a QualifierFilter (i.e. all KVs are read at the server, but none are returned to the client).

        Show
        lhofhansl Lars Hofhansl added a comment - Using a qualifier filter that just filters each qualifier (to eliminate variance from the network and the driver client), I see 7.30ms vs 7.65ms per, a ~4% improvement. This was a repeated get of a row with 200.000 columns, all of which are filtered by a QualifierFilter (i.e. all KVs are read at the server, but none are returned to the client).
        Hide
        lhofhansl Lars Hofhansl added a comment -

        I will admit that the improvement is hard to separate from the GC noise. According to the profiler it should save a about 5%.

        When I average out the scan times on the client and let it run for a while I do see about 2% improvement... Hmm. (note this for v4, with no additional new caching in KeyValue)

        I will also try with a filter that filters everything to eliminate variance in the driving client.

        Show
        lhofhansl Lars Hofhansl added a comment - I will admit that the improvement is hard to separate from the GC noise. According to the profiler it should save a about 5%. When I average out the scan times on the client and let it run for a while I do see about 2% improvement... Hmm. (note this for v4, with no additional new caching in KeyValue) I will also try with a filter that filters everything to eliminate variance in the driving client.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12541708/6621-0.96-v4.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. 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 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        -1 javac. The applied patch generated 5 javac compiler warnings (more than the trunk's current 4 warnings).

        -1 findbugs. The patch appears to introduce 7 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12541708/6621-0.96-v4.txt against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. 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 hadoop2.0. The patch compiles against the hadoop 2.0 profile. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 5 javac compiler warnings (more than the trunk's current 4 warnings). -1 findbugs. The patch appears to introduce 7 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2635//console This message is automatically generated.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        @Todd: I assume you are referring to v3 of the patch? (the part where I suggest caching the type of the KV and a new byte member) I had the same about this just being a bogus profiler indicator.

        v4 (and v2) does not cache information, it just uses the information that the HFileScanner already collected anyway (length and key length) and avoids calculating it again - it cannot make things worse. I saw a real life improvement with it. I'll quantify it.

        Show
        lhofhansl Lars Hofhansl added a comment - @Todd: I assume you are referring to v3 of the patch? (the part where I suggest caching the type of the KV and a new byte member) I had the same about this just being a bogus profiler indicator. v4 (and v2) does not cache information, it just uses the information that the HFileScanner already collected anyway (length and key length) and avoids calculating it again - it cannot make things worse. I saw a real life improvement with it. I'll quantify it.
        Hide
        tlipcon Todd Lipcon added a comment -

        Do you have benchmark results showing an improvement in actual scan speed?

        When I looked into scan performance with oprofile a few months back, I found the same as you – that a lot of time was spent in these calls. But when I also added cache miss counters to the profile, I found the reason was cache misses, not the actual CPU usage of the function. So caching them would just shift around the cache miss to the next access of the cache line elsewhere, without actually improving total performance.

        Show
        tlipcon Todd Lipcon added a comment - Do you have benchmark results showing an improvement in actual scan speed? When I looked into scan performance with oprofile a few months back, I found the same as you – that a lot of time was spent in these calls. But when I also added cache miss counters to the profile, I found the reason was cache misses, not the actual CPU usage of the function. So caching them would just shift around the cache miss to the next access of the cache line elsewhere, without actually improving total performance.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        I doubt this is going to break anything, but let's get a hadoop qa run.

        Show
        lhofhansl Lars Hofhansl added a comment - I doubt this is going to break anything, but let's get a hadoop qa run.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        v4 is like v2 with Ted's comments addressed (the note caution in the comment was pointless, since the same would happen when the wrong length is specified).

        Show
        lhofhansl Lars Hofhansl added a comment - v4 is like v2 with Ted's comments addressed (the note caution in the comment was pointless, since the same would happen when the wrong length is specified).
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Ahh, will correct the spelling. The caution is that if you pass in the wrong length of the key you'll get unpredictable results.

        Show
        lhofhansl Lars Hofhansl added a comment - Ahh, will correct the spelling. The caution is that if you pass in the wrong length of the key you'll get unpredictable results.
        Hide
        zhihyu@ebaysf.com Ted Yu added a comment -
        +   * for length <code>length</code>, and a know <code>keyLength</code>.
        

        'know' -> 'known'

        +   * Use with caution.
        

        Can you tell us what caution should be taken ?

        Show
        zhihyu@ebaysf.com Ted Yu added a comment - + * for length <code>length</code>, and a know <code>keyLength</code>. 'know' -> 'known' + * Use with caution. Can you tell us what caution should be taken ?
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Thinking now that maybe the v3 portion should be separate as it actually changes the storage patterns of KeyValue. The changes in v2 cannot lead to worse performance than before in any scenario.

        If there are no objections I'll commit v2 soon (after Stack confirmed that he in fact +1'ed v2 )

        Show
        lhofhansl Lars Hofhansl added a comment - Thinking now that maybe the v3 portion should be separate as it actually changes the storage patterns of KeyValue. The changes in v2 cannot lead to worse performance than before in any scenario. If there are no objections I'll commit v2 soon (after Stack confirmed that he in fact +1'ed v2 )
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Adds the caching of type.
        (If that should be a different change, we can just integrate v2 here)

        Show
        lhofhansl Lars Hofhansl added a comment - Adds the caching of type. (If that should be a different change, we can just integrate v2 here)
        Hide
        lhofhansl Lars Hofhansl added a comment -

        One more observation. KeyValue.getType() is called a lot. Caching that byte provides a nice benefit. In my test is tickles another few percent CPU away.
        ScannerV2.

        {next|getKeyValue|readKeyValueLen}

        now represent a large percentage of the overall CPU (about 60%), which is the way we want it.

        I'll add that to the patch.

        @Stack: Did you look at the initial patch or V2?

        Show
        lhofhansl Lars Hofhansl added a comment - One more observation. KeyValue.getType() is called a lot. Caching that byte provides a nice benefit. In my test is tickles another few percent CPU away. ScannerV2. {next|getKeyValue|readKeyValueLen} now represent a large percentage of the overall CPU (about 60%), which is the way we want it. I'll add that to the patch. @Stack: Did you look at the initial patch or V2?
        Hide
        stack stack added a comment -

        +1 on patch. Simple.

        Show
        stack stack added a comment - +1 on patch. Simple.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Combined patch to make use of the information the HScannerV2 already has.

        Show
        lhofhansl Lars Hofhansl added a comment - Combined patch to make use of the information the HScannerV2 already has.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        With these two changes Bytes.toInt is no longer the top 40 (or so) methods. (used to be in the top ten)

        Show
        lhofhansl Lars Hofhansl added a comment - With these two changes Bytes.toInt is no longer the top 40 (or so) methods. (used to be in the top ten)
        Hide
        lhofhansl Lars Hofhansl added a comment -

        The 2nd observation is that since we also know the keyLength of the KV already in HFileReaderV2, we might as well pass it to the created KV, further reducing calls to Bytes.toInt().

        Show
        lhofhansl Lars Hofhansl added a comment - The 2nd observation is that since we also know the keyLength of the KV already in HFileReaderV2, we might as well pass it to the created KV, further reducing calls to Bytes.toInt().
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Simple patch.
        currKeyLen and currValueLen are up to date (and they are in fact used in the getKey() and getValue() methods.
        This patch uses them in getKeyValue() as well, and hence avoids two Bytes.toInt conversion for each read KV during scanning.

        Show
        lhofhansl Lars Hofhansl added a comment - Simple patch. currKeyLen and currValueLen are up to date (and they are in fact used in the getKey() and getValue() methods. This patch uses them in getKeyValue() as well, and hence avoids two Bytes.toInt conversion for each read KV during scanning.

          People

          • Assignee:
            lhofhansl Lars Hofhansl
            Reporter:
            lhofhansl Lars Hofhansl
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development