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

Avoid synchronization in HRegionScannerImpl.isFilterDone

    Details

    • Hadoop Flags:
      Reviewed

      Description

      A while ago I introduced HRegoinScannerImpl.nextRaw() to allow coprocessors and scanners with caching > 1 to avoid repeated synchronization during scanning (which puts up memory fences, which in turn slows things down on multi core machines).

      Looking at the code again I see that isFilterDone() is called from nextRaw() and isFilterDone() is synchronized.
      The caller of nextRaw is required to ensure single threaded access to nextRaw() anyway, we can call an unsynchronized internal version of isFilterDone().

      1. 10117-trunk-v2.txt
        1 kB
        Lars Hofhansl
      2. 10117-trunk.txt
        2 kB
        Lars Hofhansl
      3. 10117-0.94-v3.txt
        0.8 kB
        Lars Hofhansl
      4. 10117-0.94-v2.txt
        3 kB
        Lars Hofhansl
      5. 10117-0.94.txt
        2 kB
        Lars Hofhansl

        Activity

        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in hbase-0.96-hadoop2 #147 (See https://builds.apache.org/job/hbase-0.96-hadoop2/147/)
        HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549922)

        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in hbase-0.96-hadoop2 #147 (See https://builds.apache.org/job/hbase-0.96-hadoop2/147/ ) HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549922) /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in HBase-0.98-on-Hadoop-1.1 #6 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/6/)
        HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549921)

        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in HBase-0.98-on-Hadoop-1.1 #6 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/6/ ) HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549921) /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK-on-Hadoop-1.1 #4 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/4/)
        HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549920)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK-on-Hadoop-1.1 #4 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/4/ ) HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549920) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Numbers... 20m rows, 1 col. simple select count(1) ...

        without patch with patch with patch + Phoenix patch
        8.8s 8.1s 7.4s
        Show
        lhofhansl Lars Hofhansl added a comment - Numbers... 20m rows, 1 col. simple select count(1) ... without patch with patch with patch + Phoenix patch 8.8s 8.1s 7.4s
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in hbase-0.96 #221 (See https://builds.apache.org/job/hbase-0.96/221/)
        HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549922)

        • /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in hbase-0.96 #221 (See https://builds.apache.org/job/hbase-0.96/221/ ) HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549922) /hbase/branches/0.96/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in HBase-0.98 #7 (See https://builds.apache.org/job/HBase-0.98/7/)
        HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549921)

        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in HBase-0.98 #7 (See https://builds.apache.org/job/HBase-0.98/7/ ) HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549921) /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK #4719 (See https://builds.apache.org/job/HBase-TRUNK/4719/)
        HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549920)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #4719 (See https://builds.apache.org/job/HBase-TRUNK/4719/ ) HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549920) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in HBase-0.94-security #356 (See https://builds.apache.org/job/HBase-0.94-security/356/)
        HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549917)

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in HBase-0.94-security #356 (See https://builds.apache.org/job/HBase-0.94-security/356/ ) HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549917) /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in HBase-0.94 #1223 (See https://builds.apache.org/job/HBase-0.94/1223/)
        HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549917)

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in HBase-0.94 #1223 (See https://builds.apache.org/job/HBase-0.94/1223/ ) HBASE-10117 Avoid synchronization in HRegionScannerImpl.isFilterDone (larsh: rev 1549917) /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Committed to all 4 branches. Thanks for the review.

        Show
        lhofhansl Lars Hofhansl added a comment - Committed to all 4 branches. Thanks for the review.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Ran TestSplitTransactionOnCluster with patch and it passed.

        +1

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Ran TestSplitTransactionOnCluster with patch and it passed. +1
        Hide
        ndimiduk Nick Dimiduk added a comment -

        Nice one Lars.

        Show
        ndimiduk Nick Dimiduk added a comment - Nice one Lars.
        Hide
        stack stack added a comment -

        +1 on change.

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

        The test failure is unrelated. Will commit this afternoon. (the Phoenix change is already integrated )

        Show
        lhofhansl Lars Hofhansl added a comment - The test failure is unrelated. Will commit this afternoon. (the Phoenix change is already integrated )
        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/12618003/10117-trunk-v2.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 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

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

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 2 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 patch appears to cause mvn site goal to fail.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.regionserver.TestSplitTransactionOnCluster

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//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/12618003/10117-trunk-v2.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 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 2 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 patch appears to cause mvn site goal to fail. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.regionserver.TestSplitTransactionOnCluster Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8117//console This message is automatically generated.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Was thinking that in next() (0.94) we remove the scanner from the set of current scanners; when next() is done we'd put it back. That way all synchronization is handled by the "scanners" map, and we can still ensure that no two client threads can ever use the same scanner at the same time.

        Would be a bit tricky to reason about leases then (a lease could expire before we put the scanner back into the scanners map).

        That all said, if a scanner is used with scanner caching > 1 and this simple change is committed I do not expect any great improvements from removing synchronization from the remaining methods.

        Show
        lhofhansl Lars Hofhansl added a comment - Was thinking that in next() (0.94) we remove the scanner from the set of current scanners; when next() is done we'd put it back. That way all synchronization is handled by the "scanners" map, and we can still ensure that no two client threads can ever use the same scanner at the same time. Would be a bit tricky to reason about leases then (a lease could expire before we put the scanner back into the scanners map). That all said, if a scanner is used with scanner caching > 1 and this simple change is committed I do not expect any great improvements from removing synchronization from the remaining methods.
        Hide
        stack stack added a comment -

        Now, why is RegionScannerImpl synchronized at all? Do we ever have multiple client threads at the same time call next() on the same RegionScanner?

        You the man Lars Hofhansl Can we do anything to ensure multiple threads from same client is just not possible?

        Show
        stack stack added a comment - Now, why is RegionScannerImpl synchronized at all? Do we ever have multiple client threads at the same time call next() on the same RegionScanner? You the man Lars Hofhansl Can we do anything to ensure multiple threads from same client is just not possible?
        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/12617988/10117-trunk.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 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

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

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 2 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 patch appears to cause mvn site goal to fail.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//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/12617988/10117-trunk.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 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 2 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 patch appears to cause mvn site goal to fail. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8116//console This message is automatically generated.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Same for trunk.

        I assume trunk-v2 and 0.94-v3 are not controversial (not API or assumptions are changed), so I'll just commit this tomorrow.

        Show
        lhofhansl Lars Hofhansl added a comment - Same for trunk. I assume trunk-v2 and 0.94-v3 are not controversial (not API or assumptions are changed), so I'll just commit this tomorrow.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Much cleaner patch. Keeps all changes local to RegionScannerImpl.
        Only gives a 10% improvement in Phoenix as Phoenix unnecessarily calls isFilterDone in many places (will file a separate bug there).

        Show
        lhofhansl Lars Hofhansl added a comment - Much cleaner patch. Keeps all changes local to RegionScannerImpl. Only gives a 10% improvement in Phoenix as Phoenix unnecessarily calls isFilterDone in many places (will file a separate bug there).
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Same for trunk.

        Show
        lhofhansl Lars Hofhansl added a comment - Same for trunk.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Slightly better. Don't need to synchronize while we allocate the Result[].

        Now, why is RegionScannerImpl synchronized at all? Do we ever have multiple client threads at the same time call next() on the same RegionScanner?

        Show
        lhofhansl Lars Hofhansl added a comment - Slightly better. Don't need to synchronize while we allocate the Result[]. Now, why is RegionScannerImpl synchronized at all? Do we ever have multiple client threads at the same time call next() on the same RegionScanner?
        Hide
        lhofhansl Lars Hofhansl added a comment -

        0.94 patch.
        Speeds up a Phoenix count(1) query on a tall table by 20-25%.

        Show
        lhofhansl Lars Hofhansl added a comment - 0.94 patch. Speeds up a Phoenix count(1) query on a tall table by 20-25%.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development