HBase
  1. HBase
  2. HBASE-11219

HRegionServer#createRegionLoad() should reuse RegionLoad.Builder instance when called in a loop

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.99.0, 0.98.3
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      As Andrew showed in the attachment to HBASE-11165, ClusterStatusProtos$RegionLoad$Builder took 25MB heap space.

      buildServerLoad() calls createRegionLoad() for each region on the region server.
      One instance of ClusterStatusProtos$RegionLoad$Builder can be created outside the loop and passed to createRegionLoad().

      Thanks Andrew for the finding.

      1. 11219-v1.txt
        2 kB
        Ted Yu
      2. 11219-v2.txt
        3 kB
        Ted Yu

        Activity

        Hide
        Ted Yu added a comment -

        Please comment on patch v1.

        If the approach is good, same technique can be applied to RegionSpecifier.Builder.

        Show
        Ted Yu added a comment - Please comment on patch v1. If the approach is good, same technique can be applied to RegionSpecifier.Builder.
        Hide
        Mikhail Antonov added a comment -
        if (regionLoadBldr == null) {
              regionLoadBldr = RegionLoad.newBuilder();
            }
        

        May be instead this null check just create builder always on caller side (2 places) in pass it in, rather than assign method param if null?

        Show
        Mikhail Antonov added a comment - if (regionLoadBldr == null ) { regionLoadBldr = RegionLoad.newBuilder(); } May be instead this null check just create builder always on caller side (2 places) in pass it in, rather than assign method param if null?
        Hide
        Mikhail Antonov added a comment -

        Hm..Actually, those builders are thread-local objects, so why aren't they collected?

        Show
        Mikhail Antonov added a comment - Hm..Actually, those builders are thread-local objects, so why aren't they collected?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12645935/11219-v1.txt
        against trunk revision .
        ATTACHMENT ID: 12645935

        +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 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.client.TestHCM

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12645935/11219-v1.txt against trunk revision . ATTACHMENT ID: 12645935 +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 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 1 warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.client.TestHCM Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9563//console This message is automatically generated.
        Hide
        Mikhail Antonov added a comment -

        I meant method-local, of course - sorry. I think reducing number of allocations is good (yet I don't know how big win it is here, as seems these buffers should be collected in young gen anyway?)

        Show
        Mikhail Antonov added a comment - I meant method-local, of course - sorry. I think reducing number of allocations is good (yet I don't know how big win it is here, as seems these buffers should be collected in young gen anyway?)
        Hide
        Mikhail Antonov added a comment -

        *patch looks good to me

        Show
        Mikhail Antonov added a comment - *patch looks good to me
        Hide
        Andrew Purtell added a comment -

        yet I don't know how big win it is here, as seems these buffers should be collected in young gen anyway?

        You would think but I ran GC manually before dumping the data in the attachment to HBASE-11165.

        If the approach is good, same technique can be applied to RegionSpecifier.Builder.

        Looks like an improvement, can we do the same for this other builder in one patch on this issue? Then +1 for trunk and 0.98.

        Show
        Andrew Purtell added a comment - yet I don't know how big win it is here, as seems these buffers should be collected in young gen anyway? You would think but I ran GC manually before dumping the data in the attachment to HBASE-11165 . If the approach is good, same technique can be applied to RegionSpecifier.Builder. Looks like an improvement, can we do the same for this other builder in one patch on this issue? Then +1 for trunk and 0.98.
        Hide
        Ted Yu added a comment -

        javadoc warning was not related:

        [WARNING] Javadoc Warnings
        [WARNING] javadoc: warning - Multiple sources of package comments found for package "org.apache.hadoop.hbase.io.hfile"
        

        Patch v2 applies same technique to RegionSpecifier.Builder

        Show
        Ted Yu added a comment - javadoc warning was not related: [WARNING] Javadoc Warnings [WARNING] javadoc: warning - Multiple sources of package comments found for package "org.apache.hadoop.hbase.io.hfile" Patch v2 applies same technique to RegionSpecifier.Builder
        Hide
        Andrew Purtell added a comment -

        Patch v2 looks good. Can you quantify the impact?

        Show
        Andrew Purtell added a comment - Patch v2 looks good. Can you quantify the impact?
        Hide
        Ted Yu added a comment -

        I am using jmap to check the region servers in cluster accessible to me.
        Will report back.

        Show
        Ted Yu added a comment - I am using jmap to check the region servers in cluster accessible to me. Will report back.
        Hide
        Ted Yu added a comment -

        Here is what I did:
        1. on region server running unpatched 0.98, issue 'jmap -histo' several times. I got:

        num       #instances    #bytes  Class description
        --------------------------------------------------------------------------
        21:             1994    239280  org.apache.hadoop.hbase.protobuf.generated.ClusterStatusProtos$RegionLoad$Builder
        22:             1994    239280  org.apache.hadoop.hbase.protobuf.generated.ClusterStatusProtos$RegionLoad
        

        2. stop region server
        3. deploy patched 0.98
        4. start region server
        5. issue 'jmap -histo' several times. I got:

        31:             1344    64512   java.util.HashMap
        32:             529     63480   org.apache.hadoop.hbase.protobuf.generated.ClusterStatusProtos$RegionLoad
        33:             918     58752   java.net.URL
        

        ClusterStatusProtos$RegionLoad$Builder didn't even show up in the new report.

        Show
        Ted Yu added a comment - Here is what I did: 1. on region server running unpatched 0.98, issue 'jmap -histo' several times. I got: num #instances #bytes Class description -------------------------------------------------------------------------- 21: 1994 239280 org.apache.hadoop.hbase.protobuf.generated.ClusterStatusProtos$RegionLoad$Builder 22: 1994 239280 org.apache.hadoop.hbase.protobuf.generated.ClusterStatusProtos$RegionLoad 2. stop region server 3. deploy patched 0.98 4. start region server 5. issue 'jmap -histo' several times. I got: 31: 1344 64512 java.util.HashMap 32: 529 63480 org.apache.hadoop.hbase.protobuf.generated.ClusterStatusProtos$RegionLoad 33: 918 58752 java.net.URL ClusterStatusProtos$RegionLoad$Builder didn't even show up in the new report.
        Hide
        Andrew Purtell added a comment -

        Did you GC before the jmap? Anyway, +1, it's an improvement in any case.

        Show
        Andrew Purtell added a comment - Did you GC before the jmap? Anyway, +1, it's an improvement in any case.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12646041/11219-v2.txt
        against trunk revision .
        ATTACHMENT ID: 12646041

        +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 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

        +1 lineLengths. The patch does not introduce lines longer than 100

        +1 site. The mvn site goal succeeds with this patch.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.client.TestHCM

        -1 core zombie tests. There are 1 zombie test(s): at org.apache.hadoop.hbase.mapreduce.TestTableMapReduceBase.testMultiRegionTable(TestTableMapReduceBase.java:96)

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12646041/11219-v2.txt against trunk revision . ATTACHMENT ID: 12646041 +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 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 1 warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.client.TestHCM -1 core zombie tests . There are 1 zombie test(s): at org.apache.hadoop.hbase.mapreduce.TestTableMapReduceBase.testMultiRegionTable(TestTableMapReduceBase.java:96) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9564//console This message is automatically generated.
        Hide
        Andrew Purtell added a comment -

        Test failures are unrelated. They have been showing up in multiple precommit builds, we need to clean up trunk.

        Show
        Andrew Purtell added a comment - Test failures are unrelated. They have been showing up in multiple precommit builds, we need to clean up trunk.
        Hide
        Ted Yu added a comment -

        Integrated to 0.98 and master branches.

        Thanks for the review, Andy.

        Show
        Ted Yu added a comment - Integrated to 0.98 and master branches. Thanks for the review, Andy.
        Hide
        Enis Soztutar added a comment -

        Closing this issue after 0.99.0 release.

        Show
        Enis Soztutar added a comment - Closing this issue after 0.99.0 release.

          People

          • Assignee:
            Ted Yu
            Reporter:
            Ted Yu
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development