Uploaded image for project: 'HBase'
  1. HBase
  2. HBASE-6603

RegionMetricsStorage.incrNumericMetric is called too often

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.2
    • Component/s: Performance
    • Labels:
      None

      Description

      Running an HBase scan load through the profiler revealed that RegionMetricsStorage.incrNumericMetric is called way too often.

      It turns out that we make this call for each KV in StoreScanner.next(...).
      Incrementing AtomicLong requires expensive memory barriers.

      The observation here is that StoreScanner.next(...) can maintain a simple
      long in its internal loop and only update the metric upon exit. Thus the AtomicLong is not updated nearly as often.

      That cuts about 10% runtime from scan only load (I'll quantify this better soon).

      1. 6503-0.96.txt
        7 kB
        Lars Hofhansl
      2. 6603-0.94.txt
        5 kB
        Lars Hofhansl

        Activity

        Hide
        eclark Elliott Clark added a comment -

        wow that's a huge perf gain.
        Thanks.

        Show
        eclark Elliott Clark added a comment - wow that's a huge perf gain. Thanks.
        Hide
        kannanm Kannan Muthukkaruppan added a comment -

        Also mentioned as item #4 in https://issues.apache.org/jira/browse/HBASE-6066. I think a patch for this is already out. Let me check.

        Show
        kannanm Kannan Muthukkaruppan added a comment - Also mentioned as item #4 in https://issues.apache.org/jira/browse/HBASE-6066 . I think a patch for this is already out. Let me check.
        Hide
        kannanm Kannan Muthukkaruppan added a comment -

        Item #4 in HBASE-6066 was forked off into HBASE-6217.

        Show
        kannanm Kannan Muthukkaruppan added a comment - Item #4 in HBASE-6066 was forked off into HBASE-6217 .
        Hide
        lhofhansl Lars Hofhansl added a comment -

        I should say that I tested with very wide rows. With narrow rows the percentage gain is probably less.

        Show
        lhofhansl Lars Hofhansl added a comment - I should say that I tested with very wide rows. With narrow rows the percentage gain is probably less.
        Hide
        stack stack added a comment -

        This is a dup of hbase-6217 then?

        Show
        stack stack added a comment - This is a dup of hbase-6217 then?
        Hide
        lhofhansl Lars Hofhansl added a comment -

        The patch in HBASE-6217 is similar to the change that I had made locally. (I did not add a try/finally block, but instead updated the metric at the spots where the method returns. But it's the same in principle)

        Damn, and I was so proud of my profiling detective work.

        So, should I close this as dup and mark HBASE-6217 against 0.94 and 0.96 also? Or leave HBASE-6217 against fb89 only and use this for the 0.94 and 0.96 patches?

        Show
        lhofhansl Lars Hofhansl added a comment - The patch in HBASE-6217 is similar to the change that I had made locally. (I did not add a try/finally block, but instead updated the metric at the spots where the method returns. But it's the same in principle) Damn, and I was so proud of my profiling detective work. So, should I close this as dup and mark HBASE-6217 against 0.94 and 0.96 also? Or leave HBASE-6217 against fb89 only and use this for the 0.94 and 0.96 patches?
        Hide
        stack stack added a comment -

        Damn, and I was so proud of my profiling detective work.

        You impressed me.

        ...or leave HBASE-6217 against fb89 only and use this for the 0.94 and 0.96 patches?

        That makes sense I'd say. Easier to track (having this issue for trunk and 0.94 and hbase-6217 for 89fb)

        Show
        stack stack added a comment - Damn, and I was so proud of my profiling detective work. You impressed me. ...or leave HBASE-6217 against fb89 only and use this for the 0.94 and 0.96 patches? That makes sense I'd say. Easier to track (having this issue for trunk and 0.94 and hbase-6217 for 89fb)
        Hide
        kannanm Kannan Muthukkaruppan added a comment -

        but any reason the same patch (as 89fb) wouldn't work for trunk as well? I was checking into 89fb tree yet, and looks like the change hasn't been committed there either. Following up with Michael Chen.

        Show
        kannanm Kannan Muthukkaruppan added a comment - but any reason the same patch (as 89fb) wouldn't work for trunk as well? I was checking into 89fb tree yet, and looks like the change hasn't been committed there either. Following up with Michael Chen.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Sure. Just make a 0.94/0.96 patch there. Patch there looks good to me. (copykv is gone now, though).

        Show
        lhofhansl Lars Hofhansl added a comment - Sure. Just make a 0.94/0.96 patch there. Patch there looks good to me. (copykv is gone now, though).
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Here's a version of the HBASE-6217 patch for trunk.

        In an extreme test with Gets of 60.000 columns the time went from about 164ms/iteration to 124ms.

        Show
        lhofhansl Lars Hofhansl added a comment - Here's a version of the HBASE-6217 patch for trunk. In an extreme test with Gets of 60.000 columns the time went from about 164ms/iteration to 124ms.
        Hide
        stack stack added a comment -

        +1

        Show
        stack stack added a comment - +1
        Hide
        lhofhansl Lars Hofhansl added a comment -

        OK... Going to commit soon. @Kannan: This is essentially the same patch as in HBASE-6217. I'll attribute it to M.Chen.

        Show
        lhofhansl Lars Hofhansl added a comment - OK... Going to commit soon. @Kannan: This is essentially the same patch as in HBASE-6217 . I'll attribute it to M.Chen.
        Hide
        enis Enis Soztutar added a comment -

        Great find. Can we merge to 0.92 as well.

        Show
        enis Enis Soztutar added a comment - Great find. Can we merge to 0.92 as well.
        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
        lhofhansl Lars Hofhansl added a comment -

        Turns out that 0.92 does not collect this metric, so backport is not needed.

        Show
        lhofhansl Lars Hofhansl added a comment - Turns out that 0.92 does not collect this metric, so backport is not needed.
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-0.94 #410 (See https://builds.apache.org/job/HBase-0.94/410/)
        HBASE-6603 RegionMetricsStorage.incrNumericMetric is called too often (M. Chen and Lars H) (Revision 1375318)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        Show
        hudson Hudson added a comment - Integrated in HBase-0.94 #410 (See https://builds.apache.org/job/HBase-0.94/410/ ) HBASE-6603 RegionMetricsStorage.incrNumericMetric is called too often (M. Chen and Lars H) (Revision 1375318) Result = FAILURE larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-TRUNK #3246 (See https://builds.apache.org/job/HBase-TRUNK/3246/)
        HBASE-6603 RegionMetricsStorage.incrNumericMetric is called too often (M. Chen) (Revision 1375312)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        Show
        hudson Hudson added a comment - Integrated in HBase-TRUNK #3246 (See https://builds.apache.org/job/HBase-TRUNK/3246/ ) HBASE-6603 RegionMetricsStorage.incrNumericMetric is called too often (M. Chen) (Revision 1375312) Result = FAILURE larsh : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        Hide
        kannanm Kannan Muthukkaruppan added a comment -

        Thanks Lars.

        Show
        kannanm Kannan Muthukkaruppan added a comment - Thanks Lars.
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #140 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/140/)
        HBASE-6603 RegionMetricsStorage.incrNumericMetric is called too often (M. Chen) (Revision 1375312)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        Show
        hudson Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #140 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/140/ ) HBASE-6603 RegionMetricsStorage.incrNumericMetric is called too often (M. Chen) (Revision 1375312) Result = FAILURE larsh : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Also note that for scans with few columns the collection of this metric still shows up and consumes a significant portion of the time.
        Are these metric that useful (total get size, total next size)? Can we remove them (or at least optionally not collect them)?

        Show
        lhofhansl Lars Hofhansl added a comment - Also note that for scans with few columns the collection of this metric still shows up and consumes a significant portion of the time. Are these metric that useful (total get size, total next size)? Can we remove them (or at least optionally not collect them)?
        Hide
        stack stack added a comment -

        @Lars Thats another issue?

        Show
        stack stack added a comment - @Lars Thats another issue?
        Hide
        lhofhansl Lars Hofhansl added a comment -

        @Stack: For sure, just wanted to gauge what the general opinion is.

        Show
        lhofhansl Lars Hofhansl added a comment - @Stack: For sure, just wanted to gauge what the general opinion is.
        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-6603 RegionMetricsStorage.incrNumericMetric is called too often (M. Chen and Lars H) (Revision 1375318)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.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-6603 RegionMetricsStorage.incrNumericMetric is called too often (M. Chen and Lars H) (Revision 1375318) Result = FAILURE larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        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-6603 RegionMetricsStorage.incrNumericMetric is called too often (M. Chen and Lars H) (Revision 1375318)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.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-6603 RegionMetricsStorage.incrNumericMetric is called too often (M. Chen and Lars H) (Revision 1375318) Result = FAILURE larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
        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)

          People

          • Assignee:
            mycnyc M. Chen
            Reporter:
            lhofhansl Lars Hofhansl
          • Votes:
            0 Vote for this issue
            Watchers:
            10 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development