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

Severe performance downgrade when parallel reading a single key from BucketCache

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.98.14, 1.1.2
    • Fix Version/s: 2.0.0, 1.2.0, 1.3.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      We store feature data of online items in HBase, do machine learning on these features, and supply the outputs to our online search engine. In such scenario we will launch hundreds of yarn workers and each worker will read all features of one item(i.e. single rowkey in HBase), so there'll be heavy parallel reading on a single rowkey.

      We were using LruCache but start to try BucketCache recently to resolve gc issue, and just as titled we have observed severe performance downgrade. After some analytics we found the root cause is the lock in BucketCache#getBlock, as shown below

            try {
              lockEntry = offsetLock.getLockEntry(bucketEntry.offset());
              // ...
              if (bucketEntry.equals(backingMap.get(key))) {
                // ...
                int len = bucketEntry.getLength();
                Cacheable cachedBlock = ioEngine.read(bucketEntry.offset(), len,
                    bucketEntry.deserializerReference(this.deserialiserMap));
      

      Since ioEnging.read involves array copy, it's much more time-costed than the operation in LruCache. And since we're using synchronized in IdLock#getLockEntry, parallel read dropping on the same bucket would be executed in serial, which causes a really bad performance.

      To resolve the problem, we propose to use ReentranceReadWriteLock in BucketCache, and introduce a new class called IdReadWriteLock to implement it.

      1. 14463-branch-1-v12.txt
        17 kB
        Ted Yu
      2. GC_with_WeakObjectPool.png
        124 kB
        Yu Li
      3. HBASE-14463_v11.patch
        19 kB
        Yu Li
      4. HBASE-14463_v12.patch
        18 kB
        Yu Li
      5. HBASE-14463_v12.patch
        18 kB
        Yu Li
      6. HBASE-14463_v2.patch
        15 kB
        Yu Li
      7. HBASE-14463_v3.patch
        16 kB
        Yu Li
      8. HBASE-14463_v4.patch
        16 kB
        Yu Li
      9. HBASE-14463_v5.patch
        17 kB
        Yu Li
      10. HBASE-14463.branch-0.98.patch
        28 kB
        Yu Li
      11. HBASE-14463.patch
        16 kB
        Yu Li
      12. pe_use_same_keys.patch
        6 kB
        Yu Li
      13. TestBucketCache_with_IdLock.png
        235 kB
        Yu Li
      14. TestBucketCache_with_IdLock-latest.png
        239 kB
        Yu Li
      15. TestBucketCache_with_IdReadWriteLock.png
        227 kB
        Yu Li
      16. TestBucketCache_with_IdReadWriteLock-latest.png
        236 kB
        Yu Li
      17. TestBucketCache_with_IdReadWriteLock-resolveLockLeak.png
        233 kB
        Yu Li
      18. TestBucketCache-new_with_IdLock.png
        237 kB
        Yu Li
      19. TestBucketCache-new_with_IdReadWriteLock.png
        233 kB
        Yu Li
      20. test-results.tar.gz
        220 kB
        Yu Li

        Issue Links

          Activity

          Hide
          carp84 Yu Li added a comment -

          OK, thanks for the follow-up Anoop Sam John!

          Andrew Purtell, whenever you want this in 98, just let me know.

          Show
          carp84 Yu Li added a comment - OK, thanks for the follow-up Anoop Sam John ! Andrew Purtell , whenever you want this in 98, just let me know.
          Hide
          anoop.hbase Anoop Sam John added a comment -

          0.98 backport is not so straight as it need HBASE-14268. No need to keep the fixed jira open. For 98 backport pls open a new jira referring to this. I will close this.

          Show
          anoop.hbase Anoop Sam John added a comment - 0.98 backport is not so straight as it need HBASE-14268 . No need to keep the fixed jira open. For 98 backport pls open a new jira referring to this. I will close this.
          Hide
          carp84 Yu Li added a comment -

          Ping, it seems the only work left is patch for 0.98 to close this JIRA.

          Andrew Purtell any comments on the attached patch for 0.98 sir? Or you prefer a full backport of HBASE-14268 before this goes in? Thanks.

          Show
          carp84 Yu Li added a comment - Ping, it seems the only work left is patch for 0.98 to close this JIRA. Andrew Purtell any comments on the attached patch for 0.98 sir? Or you prefer a full backport of HBASE-14268 before this goes in? Thanks.
          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/12771270/HBASE-14463.branch-0.98.patch
          against 0.98 branch at commit 1cbcf1175e6ce497936f12c60fb2e897833ace39.
          ATTACHMENT ID: 12771270

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

          +1 tests included. The patch appears to include 10 new or modified tests.

          +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1)

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

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

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

          -1 checkstyle. The applied patch generated 3891 checkstyle errors (more than the master's current 3890 errors).

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch.

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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16453//testReport/
          Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16453//artifact/patchprocess/newFindbugsWarnings.html
          Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16453//artifact/patchprocess/checkstyle-aggregate.html

          Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16453//artifact/patchprocess/patchJavadocWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16453//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/12771270/HBASE-14463.branch-0.98.patch against 0.98 branch at commit 1cbcf1175e6ce497936f12c60fb2e897833ace39. ATTACHMENT ID: 12771270 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 10 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 28 warning messages. -1 checkstyle . The applied patch generated 3891 checkstyle errors (more than the master's current 3890 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16453//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16453//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16453//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16453//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16453//console This message is automatically generated.
          Hide
          carp84 Yu Li added a comment -

          Patch for 0.98 which partially backports WeakObjectPool and its unit test case from HBASE-14268, to see what HadoopQA says

          Show
          carp84 Yu Li added a comment - Patch for 0.98 which partially backports WeakObjectPool and its unit test case from HBASE-14268 , to see what HadoopQA says
          Hide
          carp84 Yu Li added a comment -

          Thanks Ted Yu for help commit, and thanks all for review!

          Regarding patch for 0.98, I could prepare a patch with only WeakObjectPool but no changes in KeyLocker, or a full backport of HBASE-14268. Andrew Purtell please let me know which way you prefer. Thanks.

          Show
          carp84 Yu Li added a comment - Thanks Ted Yu for help commit, and thanks all for review! Regarding patch for 0.98, I could prepare a patch with only WeakObjectPool but no changes in KeyLocker, or a full backport of HBASE-14268 . Andrew Purtell please let me know which way you prefer. Thanks.
          Hide
          hudson Hudson added a comment -

          ABORTED: Integrated in HBase-1.3 #354 (See https://builds.apache.org/job/HBase-1.3/354/)
          HBASE-14463 Severe performance downgrade when parallel reading a single (tedyu: rev 61e2566c1cd9a83aa4e2bbb016d613ced01d7a6b)

          • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestIdReadWriteLock.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/util/IdReadWriteLock.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
          Show
          hudson Hudson added a comment - ABORTED: Integrated in HBase-1.3 #354 (See https://builds.apache.org/job/HBase-1.3/354/ ) HBASE-14463 Severe performance downgrade when parallel reading a single (tedyu: rev 61e2566c1cd9a83aa4e2bbb016d613ced01d7a6b) hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestIdReadWriteLock.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/IdReadWriteLock.java hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in HBase-1.2 #353 (See https://builds.apache.org/job/HBase-1.2/353/)
          HBASE-14463 Severe performance downgrade when parallel reading a single (tedyu: rev def0f6b6fe716db4988a5b98c837934122854006)

          • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/util/IdReadWriteLock.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestIdReadWriteLock.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in HBase-1.2 #353 (See https://builds.apache.org/job/HBase-1.2/353/ ) HBASE-14463 Severe performance downgrade when parallel reading a single (tedyu: rev def0f6b6fe716db4988a5b98c837934122854006) hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/IdReadWriteLock.java hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestIdReadWriteLock.java
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in HBase-1.2-IT #269 (See https://builds.apache.org/job/HBase-1.2-IT/269/)
          HBASE-14463 Severe performance downgrade when parallel reading a single (tedyu: rev def0f6b6fe716db4988a5b98c837934122854006)

          • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/util/IdReadWriteLock.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestIdReadWriteLock.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in HBase-1.2-IT #269 (See https://builds.apache.org/job/HBase-1.2-IT/269/ ) HBASE-14463 Severe performance downgrade when parallel reading a single (tedyu: rev def0f6b6fe716db4988a5b98c837934122854006) hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/IdReadWriteLock.java hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestIdReadWriteLock.java hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in HBase-1.3-IT #299 (See https://builds.apache.org/job/HBase-1.3-IT/299/)
          HBASE-14463 Severe performance downgrade when parallel reading a single (tedyu: rev 61e2566c1cd9a83aa4e2bbb016d613ced01d7a6b)

          • hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/util/IdReadWriteLock.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestIdReadWriteLock.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in HBase-1.3-IT #299 (See https://builds.apache.org/job/HBase-1.3-IT/299/ ) HBASE-14463 Severe performance downgrade when parallel reading a single (tedyu: rev 61e2566c1cd9a83aa4e2bbb016d613ced01d7a6b) hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/IdReadWriteLock.java hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestIdReadWriteLock.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          Integrated to branch-1.2 +

          0.98 needs backport of HBASE-14268 first

          Show
          yuzhihong@gmail.com Ted Yu added a comment - Integrated to branch-1.2 + 0.98 needs backport of HBASE-14268 first
          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/12771116/14463-branch-1-v12.txt
          against branch-1 branch at commit 263a0adf79105b9dc166e21c3f5159ade6e2d0a7.
          ATTACHMENT ID: 12771116

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

          +1 tests included. The patch appears to include 8 new or modified tests.

          +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1)

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

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

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

          +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch.

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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16438//testReport/
          Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16438//artifact/patchprocess/newFindbugsWarnings.html
          Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16438//artifact/patchprocess/checkstyle-aggregate.html

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16438//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/12771116/14463-branch-1-v12.txt against branch-1 branch at commit 263a0adf79105b9dc166e21c3f5159ade6e2d0a7. ATTACHMENT ID: 12771116 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 8 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16438//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16438//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16438//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16438//console This message is automatically generated.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in HBase-Trunk_matrix #440 (See https://builds.apache.org/job/HBase-Trunk_matrix/440/)
          HBASE-14463 Severe performance downgrade when parallel reading a single (tedyu: rev 263a0adf79105b9dc166e21c3f5159ade6e2d0a7)

          • hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestIdReadWriteLock.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java
          • hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/util/IdReadWriteLock.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in HBase-Trunk_matrix #440 (See https://builds.apache.org/job/HBase-Trunk_matrix/440/ ) HBASE-14463 Severe performance downgrade when parallel reading a single (tedyu: rev 263a0adf79105b9dc166e21c3f5159ade6e2d0a7) hbase-server/src/test/java/org/apache/hadoop/hbase/util/TestIdReadWriteLock.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java hbase-server/src/main/java/org/apache/hadoop/hbase/util/IdReadWriteLock.java
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          HBASE-14268 went into 1.2+

          For 0.98, we need to backport HBASE-14268 first

          Show
          yuzhihong@gmail.com Ted Yu added a comment - HBASE-14268 went into 1.2+ For 0.98, we need to backport HBASE-14268 first
          Hide
          anoop.hbase Anoop Sam John added a comment -

          Can you attach patches for other branches as well. We need fix this in 0.98+ versions

          Show
          anoop.hbase Anoop Sam John added a comment - Can you attach patches for other branches as well. We need fix this in 0.98+ versions
          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/12770952/HBASE-14463_v12.patch
          against master branch at commit bfa36891901b96b95d82f5307642c35fd2b9f534.
          ATTACHMENT ID: 12770952

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

          +1 tests included. The patch appears to include 8 new or modified tests.

          +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1)

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

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

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

          +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch.

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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16422//testReport/
          Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16422//artifact/patchprocess/newFindbugsWarnings.html
          Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16422//artifact/patchprocess/checkstyle-aggregate.html

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16422//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/12770952/HBASE-14463_v12.patch against master branch at commit bfa36891901b96b95d82f5307642c35fd2b9f534. ATTACHMENT ID: 12770952 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 8 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16422//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16422//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16422//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16422//console This message is automatically generated.
          Hide
          carp84 Yu Li added a comment -

          Thanks Anoop Sam John for the double check and confirmation!

          Checked and confirmed v12 patch could apply in the latest code base, re-attach to see what HadoopQA will say (with recent commits/code changes)

          Show
          carp84 Yu Li added a comment - Thanks Anoop Sam John for the double check and confirmation! Checked and confirmed v12 patch could apply in the latest code base, re-attach to see what HadoopQA will say (with recent commits/code changes)
          Hide
          anoop.hbase Anoop Sam John added a comment -

          Sorry for the delay.
          I did the tests again by fixing the RKs what we fetch (via multi get) every time. ( Not exactly this way as in patchpe_use_same_keys.patch. instead of doing a random RK generation, I can use the incoming int 'i' to testRow(final int i) to make RKs )
          Both with and with out patch gives almost similar avg total run time for threads. So in my test, I have ensured that every thread doing distinct RK fetch only. And there also we don't degrade perf.

          So here is my +1

          Thanks for the perseverance Yu Li

          Show
          anoop.hbase Anoop Sam John added a comment - Sorry for the delay. I did the tests again by fixing the RKs what we fetch (via multi get) every time. ( Not exactly this way as in patchpe_use_same_keys.patch. instead of doing a random RK generation, I can use the incoming int 'i' to testRow(final int i) to make RKs ) Both with and with out patch gives almost similar avg total run time for threads. So in my test, I have ensured that every thread doing distinct RK fetch only. And there also we don't degrade perf. So here is my +1 Thanks for the perseverance Yu Li
          Hide
          stack stack added a comment -

          Any updates on this one Anoop Sam John ? Thanks sir.

          Show
          stack stack added a comment - Any updates on this one Anoop Sam John ? Thanks sir.
          Hide
          carp84 Yu Li added a comment -

          5% down is a big number IMO. So wanted to see why we perform poor with the patch. What are the reasons for that.

          Yes, totally understand, and thanks for the double check here Anoop Sam John. Will wait for your testing result with the same random keys.

          Show
          carp84 Yu Li added a comment - 5% down is a big number IMO. So wanted to see why we perform poor with the patch. What are the reasons for that. Yes, totally understand, and thanks for the double check here Anoop Sam John . Will wait for your testing result with the same random keys.
          Hide
          anoop.hbase Anoop Sam John added a comment -

          So the most fair way is to test with the same random keys

          Yes. I can do that tonight..

          Yes with random keys, with run to run there can be different completion time.. That is why with and with out patch doing the run at least 3 times. So with out patch itself, we get slightly different times. But the deviation in not much.. 5% down is a big number IMO. So wanted to see why we perform poor with the patch. What are the reasons for that.

          Show
          anoop.hbase Anoop Sam John added a comment - So the most fair way is to test with the same random keys Yes. I can do that tonight.. Yes with random keys, with run to run there can be different completion time.. That is why with and with out patch doing the run at least 3 times. So with out patch itself, we get slightly different times. But the deviation in not much.. 5% down is a big number IMO. So wanted to see why we perform poor with the patch. What are the reasons for that.
          Hide
          carp84 Yu Li added a comment -

          I did not do the fixed row keys part as u did for PE tool

          well, the "fixed" row keys are also randomly generated by PE tool, but I just save them to file and use it to test both scenarios to avoid deviations caused by different key distribution. You could regard the change I made to PE tool as: 1. generate very random keys for read query; 2. test cluster w/ and w/o patch; 3. compare the result.

          Notice that even with the same impl, perf number in different run diverges and the gap might be as much as 3~5%, which could prove it well that random key distribution could cause fluctuation in perf number. So the most fair way is to test with the same random keys, agree Anoop Sam John?

          Show
          carp84 Yu Li added a comment - I did not do the fixed row keys part as u did for PE tool well, the "fixed" row keys are also randomly generated by PE tool, but I just save them to file and use it to test both scenarios to avoid deviations caused by different key distribution. You could regard the change I made to PE tool as: 1. generate very random keys for read query; 2. test cluster w/ and w/o patch; 3. compare the result. Notice that even with the same impl, perf number in different run diverges and the gap might be as much as 3~5%, which could prove it well that random key distribution could cause fluctuation in perf number. So the most fair way is to test with the same random keys, agree Anoop Sam John ?
          Hide
          anoop.hbase Anoop Sam John added a comment -

          I tested it again and still seeing down as mentioned above.. PE tool with multi get and 100GB data.. But I did not do the fixed row keys part as u did for PE tool.. So the tool generates very random RKs.. There might be same keys being requested by 2 threads at same time.. But it is very random... Why is this much down for that kind of a scenario? Can u measure the perf numbers with the micro benchmark test case where all RKs are different (U did it with all RKs same).. So what I say is the worst case where all RKs are different.

          Show
          anoop.hbase Anoop Sam John added a comment - I tested it again and still seeing down as mentioned above.. PE tool with multi get and 100GB data.. But I did not do the fixed row keys part as u did for PE tool.. So the tool generates very random RKs.. There might be same keys being requested by 2 threads at same time.. But it is very random... Why is this much down for that kind of a scenario? Can u measure the perf numbers with the micro benchmark test case where all RKs are different (U did it with all RKs same).. So what I say is the worst case where all RKs are different.
          Hide
          ram_krish ramkrishna.s.vasudevan added a comment -

          I think if there is no much of a degrade because of the patch then it should be fine. +1 from me too.

          Show
          ram_krish ramkrishna.s.vasudevan added a comment - I think if there is no much of a degrade because of the patch then it should be fine. +1 from me too.
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          I think the latest round of performance benchmarking shows acceptable results.

          +1 from me.

          Show
          yuzhihong@gmail.com Ted Yu added a comment - I think the latest round of performance benchmarking shows acceptable results. +1 from me.
          Hide
          carp84 Yu Li added a comment -

          Ping, any more comments here fellows?

          A summary of testing done:

          1. For the multi thread reading single key scenario, it's covered by the existing UT case TestBucketCache#testCacheMultiThreaded. The testing result shows obvious perf improvement with current patch
          2. For the more common random read case, with the same query key distribution and testing on one node real cluster, result shows no performance downgrade with current patch
          Show
          carp84 Yu Li added a comment - Ping, any more comments here fellows? A summary of testing done: For the multi thread reading single key scenario, it's covered by the existing UT case TestBucketCache#testCacheMultiThreaded. The testing result shows obvious perf improvement with current patch For the more common random read case, with the same query key distribution and testing on one node real cluster, result shows no performance downgrade with current patch
          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/12768112/pe_use_same_keys.patch
          against master branch at commit 467bc098a9512afca38356da56d92c351f15b042.
          ATTACHMENT ID: 12768112

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

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1)

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

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

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

          +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch.

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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16179//testReport/
          Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16179//artifact/patchprocess/newFindbugsWarnings.html
          Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16179//artifact/patchprocess/checkstyle-aggregate.html

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16179//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/12768112/pe_use_same_keys.patch against master branch at commit 467bc098a9512afca38356da56d92c351f15b042. ATTACHMENT ID: 12768112 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16179//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16179//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16179//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16179//console This message is automatically generated.
          Hide
          carp84 Yu Li added a comment -

          Some explanation about how to use the patched PE tool:

          First of all, run the same command as previously to generate the keys:

          bin/hbase org.apache.hadoop.hbase.PerformanceEvaluation --nomapred --multiGet=100 randomRead 25
          

          The keys will be generated in /tmp/getsForTest (a hard-coded path for now), one per line. Notice: result of this loading round should be excluded

          Then, add "--loadGets" option to the command, which will load the stored keys to form the multi gets

          bin/hbase org.apache.hadoop.hbase.PerformanceEvaluation --nomapred --multiGet=100 --loadGets randomRead 25
          
          Show
          carp84 Yu Li added a comment - Some explanation about how to use the patched PE tool: First of all, run the same command as previously to generate the keys: bin/hbase org.apache.hadoop.hbase.PerformanceEvaluation --nomapred --multiGet=100 randomRead 25 The keys will be generated in /tmp/getsForTest (a hard-coded path for now), one per line. Notice: result of this loading round should be excluded Then, add "--loadGets" option to the command, which will load the stored keys to form the multi gets bin/hbase org.apache.hadoop.hbase.PerformanceEvaluation --nomapred --multiGet=100 --loadGets randomRead 25
          Hide
          carp84 Yu Li added a comment -

          After supporting record/load keys for randomReads in PE tool (code changes refer to the attached patch), recheck the performance with --multiGet=100 and 25 threads, results are as follows:

          w/o patch:
          1. Min: 94220ms    Max: 95193ms    Avg: 94826ms
          2. Min: 91405ms    Max: 92271ms    Avg: 91955ms
          3. Min: 95314ms    Max: 96266ms    Avg: 95946ms
          4. Min: 95545ms    Max: 96534ms    Avg: 96208ms
          Average: 94733.75ms
          
          w/ patch:
          1. Min: 94887ms    Max: 95890ms    Avg: 95561ms
          2. Min: 94681ms    Max: 95643ms    Avg: 95285ms
          3. Min: 93880ms    Max: 94856ms    Avg: 94514ms
          4. Min: 93418ms    Max: 94283ms    Avg: 93981ms
          Average: 94835.25ms
          

          The correlated BucketCache status:

          w/o patch:
          1. Hits Caching    18,821,913; Misses Caching  11,595
          2. Hits Caching    18,821,913; Misses Caching  11,588
          3. Hits Caching    18,821,913; Misses Caching  11,586
          4. Hits Caching    18,821,913; Misses Caching  11,587
          
          w/ patch:
          1. Hits Caching    18,821,913; Misses Caching  11,586
          2. Hits Caching    18,821,913; Misses Caching  11,590
          3. Hits Caching    18,821,913; Misses Caching  11,587
          4. Hits Caching    18,821,913; Misses Caching  11,588
          

          We could see no more perf downgrade (~0.1%).

          Anoop Sam John, ramkrishna.s.vasudevan, Jingcheng Du, Lars Hofhansl and Hiroshi Ikeda, does this latest test result make sense to you? Or any comments? Thanks.

          Show
          carp84 Yu Li added a comment - After supporting record/load keys for randomReads in PE tool (code changes refer to the attached patch), recheck the performance with --multiGet=100 and 25 threads, results are as follows: w/o patch: 1. Min: 94220ms Max: 95193ms Avg: 94826ms 2. Min: 91405ms Max: 92271ms Avg: 91955ms 3. Min: 95314ms Max: 96266ms Avg: 95946ms 4. Min: 95545ms Max: 96534ms Avg: 96208ms Average: 94733.75ms w/ patch: 1. Min: 94887ms Max: 95890ms Avg: 95561ms 2. Min: 94681ms Max: 95643ms Avg: 95285ms 3. Min: 93880ms Max: 94856ms Avg: 94514ms 4. Min: 93418ms Max: 94283ms Avg: 93981ms Average: 94835.25ms The correlated BucketCache status: w/o patch: 1. Hits Caching 18,821,913; Misses Caching 11,595 2. Hits Caching 18,821,913; Misses Caching 11,588 3. Hits Caching 18,821,913; Misses Caching 11,586 4. Hits Caching 18,821,913; Misses Caching 11,587 w/ patch: 1. Hits Caching 18,821,913; Misses Caching 11,586 2. Hits Caching 18,821,913; Misses Caching 11,590 3. Hits Caching 18,821,913; Misses Caching 11,587 4. Hits Caching 18,821,913; Misses Caching 11,588 We could see no more perf downgrade (~0.1%). Anoop Sam John , ramkrishna.s.vasudevan , Jingcheng Du , Lars Hofhansl and Hiroshi Ikeda , does this latest test result make sense to you? Or any comments? Thanks.
          Hide
          carp84 Yu Li added a comment -

          Checked multi get with --multiGet=100, and checked both 25 threads and 50 threads, results are as follows:

          25threads:

          w/o patch:
          1. Min: 94303ms    Max: 94853ms    Avg: 94651ms
          2. Min: 92547ms    Max: 93140ms    Avg: 92934ms
          3. Min: 90562ms    Max: 91374ms    Avg: 91168ms
          Average: 92917.67ms
          
          w/ patch:
          1. Min: 94026ms    Max: 94631ms    Avg: 94417ms
          2. Min: 94086ms    Max: 94850ms    Avg: 94643ms
          3. Min: 95041ms    Max: 95585ms    Avg: 95371ms
          Average: 94810ms
          
          with slow purge(lock pool size reach 5000):
          1. Min: 94699ms    Max: 95276ms    Avg: 95031ms
          2. Min: 97087ms    Max: 98106ms    Avg: 97871ms
          3. Min: 93804ms    Max: 94743ms    Avg: 94519ms
          Average: 95807ms
          

          From which we could see there's indeed a ~2% downgrade w/ patch, and slow purge (only purge lockPool when its size reaches 5000) didn't bring any improvement

          However, with 50 threads:

          w/o patch
          1. Min: 195573ms   Max: 199973ms   Avg: 199016ms
          2. Min: 191126ms   Max: 194468ms   Avg: 193592ms
          3. Min: 195415ms   Max: 198985ms   Avg: 198133ms
          Average: 196913.67
          
          w/ patch:
          1. Min: 199279ms   Max: 202442ms   Avg: 201607ms
          2. Min: 191189ms   Max: 194442ms   Avg: 193626ms
          3. Min: 190235ms   Max: 193817ms   Avg: 192996ms
          Average: 196076.33ms
          

          I got a better result w/ patch.

          Now my suspicion is the different distribution of chosen keys makes different result, so I'm planning to test the 2 cases (w/ and w/o patch) with exactly the same randomly-generated keys and see what will happen

          Show
          carp84 Yu Li added a comment - Checked multi get with --multiGet=100, and checked both 25 threads and 50 threads, results are as follows: 25threads: w/o patch: 1. Min: 94303ms Max: 94853ms Avg: 94651ms 2. Min: 92547ms Max: 93140ms Avg: 92934ms 3. Min: 90562ms Max: 91374ms Avg: 91168ms Average: 92917.67ms w/ patch: 1. Min: 94026ms Max: 94631ms Avg: 94417ms 2. Min: 94086ms Max: 94850ms Avg: 94643ms 3. Min: 95041ms Max: 95585ms Avg: 95371ms Average: 94810ms with slow purge(lock pool size reach 5000): 1. Min: 94699ms Max: 95276ms Avg: 95031ms 2. Min: 97087ms Max: 98106ms Avg: 97871ms 3. Min: 93804ms Max: 94743ms Avg: 94519ms Average: 95807ms From which we could see there's indeed a ~2% downgrade w/ patch, and slow purge (only purge lockPool when its size reaches 5000) didn't bring any improvement However, with 50 threads: w/o patch 1. Min: 195573ms Max: 199973ms Avg: 199016ms 2. Min: 191126ms Max: 194468ms Avg: 193592ms 3. Min: 195415ms Max: 198985ms Avg: 198133ms Average: 196913.67 w/ patch: 1. Min: 199279ms Max: 202442ms Avg: 201607ms 2. Min: 191189ms Max: 194442ms Avg: 193626ms 3. Min: 190235ms Max: 193817ms Avg: 192996ms Average: 196076.33ms I got a better result w/ patch. Now my suspicion is the different distribution of chosen keys makes different result, so I'm planning to test the 2 cases (w/ and w/o patch) with exactly the same randomly-generated keys and see what will happen
          Hide
          anoop.hbase Anoop Sam John added a comment -

          I tried with multi get only but with --multiGet=100. That is the only diff.. Yes I also did the test 4 times and taken the avg.

          Show
          anoop.hbase Anoop Sam John added a comment - I tried with multi get only but with --multiGet=100. That is the only diff.. Yes I also did the test 4 times and taken the avg.
          Hide
          carp84 Yu Li added a comment -

          The micro benchmark numbers you mentioned above - That is in trunk code base?

          Yes, it's the result of TestBucketCache#testCacheMultiThreadedSingleKey, in trunk code base.

          In trunk this is no longer true

          Agree, but UT shows the change from IdLock to IdReadWriteLock still benifits

          Regarding the PE result, I tried to reproduce it on my local, also with 100GB data and multi get with 25 threads, cmdline like:

          bin/hbase org.apache.hadoop.hbase.PerformanceEvaluation --nomapred --multiGet=20 randomRead 25
          

          Each case for 3 times and get the average result to limit the deviation. And here is the result:

          before patch:
          Min: 102832ms   Max: 103637ms   Avg: 103394ms
          Min: 101003ms   Max: 101836ms   Avg: 101564ms
          Min: 101464ms   Max: 102498ms   Avg: 102254ms
          Average: 102404ms
          
          after v12 patch:
          Min: 99542ms    Max: 100141ms   Avg: 99932ms
          Min: 102927ms   Max: 103863ms   Avg: 103580ms
          Min: 104310ms   Max: 104880ms   Avg: 104683ms
          Average: 102731ms
          
          with slow purge (when lockPool size reach 500):
          Min: 100144ms   Max: 101144ms   Avg: 100871ms
          Min: 103764ms   Max: 104588ms   Avg: 104350ms
          Min: 101310ms   Max: 102244ms   Avg: 102010ms
          Average: 102410ms
          

          From which I could see a bigger stddev after patch but similar average. Refer to the attached PE output for more details.

          Anoop Sam John, is my testing the same as yours? Any difference or maybe you just ran once for each case and it happened to be best case w/o patch and worst w/ patch? Just let me know your thoughts, thanks.

          Show
          carp84 Yu Li added a comment - The micro benchmark numbers you mentioned above - That is in trunk code base? Yes, it's the result of TestBucketCache#testCacheMultiThreadedSingleKey, in trunk code base. In trunk this is no longer true Agree, but UT shows the change from IdLock to IdReadWriteLock still benifits Regarding the PE result, I tried to reproduce it on my local, also with 100GB data and multi get with 25 threads, cmdline like: bin/hbase org.apache.hadoop.hbase.PerformanceEvaluation --nomapred --multiGet=20 randomRead 25 Each case for 3 times and get the average result to limit the deviation. And here is the result: before patch: Min: 102832ms Max: 103637ms Avg: 103394ms Min: 101003ms Max: 101836ms Avg: 101564ms Min: 101464ms Max: 102498ms Avg: 102254ms Average: 102404ms after v12 patch: Min: 99542ms Max: 100141ms Avg: 99932ms Min: 102927ms Max: 103863ms Avg: 103580ms Min: 104310ms Max: 104880ms Avg: 104683ms Average: 102731ms with slow purge (when lockPool size reach 500): Min: 100144ms Max: 101144ms Avg: 100871ms Min: 103764ms Max: 104588ms Avg: 104350ms Min: 101310ms Max: 102244ms Avg: 102010ms Average: 102410ms From which I could see a bigger stddev after patch but similar average. Refer to the attached PE output for more details. Anoop Sam John , is my testing the same as yours? Any difference or maybe you just ran once for each case and it happened to be best case w/o patch and worst w/ patch? Just let me know your thoughts, thanks.
          Hide
          anoop.hbase Anoop Sam John added a comment -

          The micro benchmark numbers you mentioned above - That is in trunk code base? Also in that test all threads try to read same key. Can you test with diff cases like all try to read diff keys or a set of diff keys and change this set size and repeat the tests.

          In description u said

          Since ioEnging.read involves array copy, it's much more time-costed than the operation in LruCache. And since we're using synchronized in IdLock#getLockEntry, parallel read dropping on the same bucket would be executed in serial, which causes a really bad performance.

          In trunk this is no longer true. We will not do any copy of the bucketed data into new buffer. We will just create a wrapper data structure across 1 or more ByteBuffers.

          Show
          anoop.hbase Anoop Sam John added a comment - The micro benchmark numbers you mentioned above - That is in trunk code base? Also in that test all threads try to read same key. Can you test with diff cases like all try to read diff keys or a set of diff keys and change this set size and repeat the tests. In description u said Since ioEnging.read involves array copy, it's much more time-costed than the operation in LruCache. And since we're using synchronized in IdLock#getLockEntry, parallel read dropping on the same bucket would be executed in serial, which causes a really bad performance. In trunk this is no longer true. We will not do any copy of the bucketed data into new buffer. We will just create a wrapper data structure across 1 or more ByteBuffers.
          Hide
          carp84 Yu Li added a comment -

          Thanks all for taking a look here.

          Was trying to reproduce Anoop Sam John/ramkrishna.s.vasudevan's result and do some investigation but met with some problem, such as jvm crash during data ingestion with PE (haven't file any JIRA since not sure whether it's an env-specific issue) and AssertionError during multi get testing (see HBASE-14660). Now I could get the test run after disabling assertion and will do further debugging, will update my findings later.

          Jingcheng Du I also doubt about the purge call slows down the performance, will add some threshold there and check the perf comparison. Thanks for point it out.

          Lars Hofhansl we need to store the lock(entry) somewhere and using lockPool is for reducing lock contention. I think the idea of using weak reference is good but lack of some perf testing here before. Or any better idea please let me know

          Show
          carp84 Yu Li added a comment - Thanks all for taking a look here. Was trying to reproduce Anoop Sam John / ramkrishna.s.vasudevan 's result and do some investigation but met with some problem, such as jvm crash during data ingestion with PE (haven't file any JIRA since not sure whether it's an env-specific issue) and AssertionError during multi get testing (see HBASE-14660 ). Now I could get the test run after disabling assertion and will do further debugging, will update my findings later. Jingcheng Du I also doubt about the purge call slows down the performance, will add some threshold there and check the perf comparison. Thanks for point it out. Lars Hofhansl we need to store the lock(entry) somewhere and using lockPool is for reducing lock contention. I think the idea of using weak reference is good but lack of some perf testing here before. Or any better idea please let me know
          Hide
          ikeda Hiroshi Ikeda added a comment -

          I rather worry about using ReadWriteLock, which is heavier than simple Lock and there might be always trade-off.

          Show
          ikeda Hiroshi Ikeda added a comment - I rather worry about using ReadWriteLock, which is heavier than simple Lock and there might be always trade-off.
          Hide
          jingcheng.du@intel.com Jingcheng Du added a comment -

          It uses the WeakObjectPool, the lock can be collected by gc very fast if threads don't request the same lock. In acquireLock it has to purges the null reference from the pool every time. Maybe this slow down the performance.
          Could we just add some threshold to the purge method, for example if the cached objects are larger than a threshold, we purge, otherwise, we don't. We can add such things to WeakObjectPool I think.

          Show
          jingcheng.du@intel.com Jingcheng Du added a comment - It uses the WeakObjectPool, the lock can be collected by gc very fast if threads don't request the same lock. In acquireLock it has to purges the null reference from the pool every time. Maybe this slow down the performance. Could we just add some threshold to the purge method, for example if the cached objects are larger than a threshold, we purge, otherwise, we don't. We can add such things to WeakObjectPool I think.
          Hide
          ram_krish ramkrishna.s.vasudevan added a comment -

          If we try hitting the same block over and over will it have different type of behaviour? I doubt no. May be worth checking where is the actual time spent here. With 50 threads and default PE data size the with patch case is performing lesser. May be more the threads there is a time it has for clearing the weak references?

          Show
          ram_krish ramkrishna.s.vasudevan added a comment - If we try hitting the same block over and over will it have different type of behaviour? I doubt no. May be worth checking where is the actual time spent here. With 50 threads and default PE data size the with patch case is performing lesser. May be more the threads there is a time it has for clearing the weak references?
          Hide
          lhofhansl Lars Hofhansl added a comment -

          Curious: What's the penalty of not having the lockPool? Past experiences taught me that these things will never be sized right.

          Show
          lhofhansl Lars Hofhansl added a comment - Curious: What's the penalty of not having the lockPool? Past experiences taught me that these things will never be sized right.
          Hide
          lhofhansl Lars Hofhansl added a comment -

          Never mind. Looked at the patch again - should have taken a closer look before I sent the previous. Looks good.

          Show
          lhofhansl Lars Hofhansl added a comment - Never mind. Looked at the patch again - should have taken a closer look before I sent the previous. Looks good.
          Hide
          lhofhansl Lars Hofhansl added a comment -

          Bit late to the party. We do have the IdLock in order to only lock the block(s) in question, and not take a "global" lock in a sense. That probably causes the 5% degradation. I'd assume that'd be worse if we only hit random blocks and we have to load the blocks.

          On first blush this does not strike as the right solution.

          In HFileReaderXXX we do double-checked locking in order to avoid taking the lock completely when the block is already cached. Can we do something this that here?

          Show
          lhofhansl Lars Hofhansl added a comment - Bit late to the party. We do have the IdLock in order to only lock the block(s) in question, and not take a "global" lock in a sense. That probably causes the 5% degradation. I'd assume that'd be worse if we only hit random blocks and we have to load the blocks. On first blush this does not strike as the right solution. In HFileReaderXXX we do double-checked locking in order to avoid taking the lock completely when the block is already cached. Can we do something this that here?
          Hide
          anoop.hbase Anoop Sam John added a comment -

          On a single node cluster with 100 GB data. The whole data is loaded in to off heap bucket cache.
          Doing multi get test with performance evaluation tool. Having 25 client threads.
          The patch slows down the operation a bit. 5% or so.
          The read pattern may be such that different threads requesting for same block is rare. Still there should not be a slow down.

          Show
          anoop.hbase Anoop Sam John added a comment - On a single node cluster with 100 GB data. The whole data is loaded in to off heap bucket cache. Doing multi get test with performance evaluation tool. Having 25 client threads. The patch slows down the operation a bit. 5% or so. The read pattern may be such that different threads requesting for same block is rare. Still there should not be a slow down.
          Hide
          anoop.hbase Anoop Sam John added a comment -

          I am doing a cluster testing with this patch

          Show
          anoop.hbase Anoop Sam John added a comment - I am doing a cluster testing with this patch
          Hide
          carp84 Yu Li added a comment -

          stack boss, anything more to be addressed or could I get your +1 here? Thanks.

          Show
          carp84 Yu Li added a comment - stack boss, anything more to be addressed or could I get your +1 here? Thanks.
          Hide
          carp84 Yu Li added a comment -

          Launched several YCSB workloadc testing against a 4 nodes dev cluster with zipfian/hotspot requestdistributio, each round running for around 20 minutes and watch the GC status with VirsualVM, the result shows no memory leak in the new implementation with WeakObjectPool. See the attached screenshot for more details.

          Show
          carp84 Yu Li added a comment - Launched several YCSB workloadc testing against a 4 nodes dev cluster with zipfian/hotspot requestdistributio, each round running for around 20 minutes and watch the GC status with VirsualVM, the result shows no memory leak in the new implementation with WeakObjectPool. See the attached screenshot for more details.
          Hide
          carp84 Yu Li added a comment -

          Affter applying the changes to CacheTestUtils (add a thread to periodically evict and recache the block) and TestBucketCache (double parallel thread number and total queries number), perf number with IdLock and latest IdReadWriteLock are as follows:

          200 threads and 20,000 queries in total reading a single key
          
          w/ IdLock and blocksize=16K: 35.597s
          w/ IdReadWriteLock and blocksize=16K: 0.212s
          

          Attached is the screenshot of relative JUnit result

          Show
          carp84 Yu Li added a comment - Affter applying the changes to CacheTestUtils (add a thread to periodically evict and recache the block) and TestBucketCache (double parallel thread number and total queries number), perf number with IdLock and latest IdReadWriteLock are as follows: 200 threads and 20,000 queries in total reading a single key w/ IdLock and blocksize=16K: 35.597s w/ IdReadWriteLock and blocksize=16K: 0.212s Attached is the screenshot of relative JUnit result
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          Since the latest patch is quite different from initial patch, mind re-doing cluster testing to show the benefit ?

          Thanks

          Show
          yuzhihong@gmail.com Ted Yu added a comment - Since the latest patch is quite different from initial patch, mind re-doing cluster testing to show the benefit ? Thanks
          Hide
          carp84 Yu Li added a comment -

          The left javadoc warning is introduced by HBASE-14268 and have just uploaded an addendum patch to fix it there.

          Show
          carp84 Yu Li added a comment - The left javadoc warning is introduced by HBASE-14268 and have just uploaded an addendum patch to fix it there.
          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/12766510/HBASE-14463_v12.patch
          against master branch at commit 94bfe909aff9fd74cb1a5d0c3f9209a19704c6cf.
          ATTACHMENT ID: 12766510

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

          +1 tests included. The patch appears to include 8 new or modified tests.

          +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1)

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

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

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

          +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch.

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

          -1 core zombie tests. There are 1 zombie test(s): at org.apache.reef.io.network.NetworkServiceTest.testMessagingNetworkServiceRate(NetworkServiceTest.java:195)

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16004//testReport/
          Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16004//artifact/patchprocess/newFindbugsWarnings.html
          Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16004//artifact/patchprocess/checkstyle-aggregate.html

          Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16004//artifact/patchprocess/patchJavadocWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16004//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/12766510/HBASE-14463_v12.patch against master branch at commit 94bfe909aff9fd74cb1a5d0c3f9209a19704c6cf. ATTACHMENT ID: 12766510 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 8 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 1 warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . -1 core zombie tests . There are 1 zombie test(s): at org.apache.reef.io.network.NetworkServiceTest.testMessagingNetworkServiceRate(NetworkServiceTest.java:195) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16004//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16004//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16004//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16004//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16004//console This message is automatically generated.
          Hide
          apurtell Andrew Purtell added a comment -

          Check the javadoc warnings, otherwise the precommit result is good - the reported zombie is a false alarm.

          Show
          apurtell Andrew Purtell added a comment - Check the javadoc warnings, otherwise the precommit result is good - the reported zombie is a false alarm.
          Hide
          stack stack added a comment -

          Thanks for doing up the summary Yu Li Helps those of us trying to follow along.

          Show
          stack stack added a comment - Thanks for doing up the summary Yu Li Helps those of us trying to follow along.
          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/12766296/HBASE-14463_v11.patch
          against master branch at commit 657078b353f215ab02ff7ac2b449006090c0c971.
          ATTACHMENT ID: 12766296

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

          +1 tests included. The patch appears to include 8 new or modified tests.

          +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1)

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

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

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

          +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch.

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

          -1 core zombie tests. There are 1 zombie test(s): at org.apache.cxf.systest.ws.addr_wsdl.AddNumberImpl.execute(AddNumberImpl.java:53)
          at org.apache.cxf.systest.ws.addr_wsdl.AddNumberImpl.addNumbers(AddNumberImpl.java:39)
          at org.apache.cxf.systest.ws.addr_wsdl.AddNumberImpl.execute(AddNumberImpl.java:53)
          at org.apache.cxf.systest.ws.addr_wsdl.AddNumberImpl.addNumbers(AddNumberImpl.java:39)
          at org.apache.cxf.systest.ws.addr_wsdl.jaxwsmm.WSDLAddrPolicyAttachmentJaxwsMMProviderTest.testUsingAddressing(WSDLAddrPolicyAttachmentJaxwsMMProviderTest.java:117)

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/15984//testReport/
          Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15984//artifact/patchprocess/newFindbugsWarnings.html
          Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/15984//artifact/patchprocess/checkstyle-aggregate.html

          Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15984//artifact/patchprocess/patchJavadocWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/15984//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/12766296/HBASE-14463_v11.patch against master branch at commit 657078b353f215ab02ff7ac2b449006090c0c971. ATTACHMENT ID: 12766296 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 8 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 2 warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . -1 core zombie tests . There are 1 zombie test(s): at org.apache.cxf.systest.ws.addr_wsdl.AddNumberImpl.execute(AddNumberImpl.java:53) at org.apache.cxf.systest.ws.addr_wsdl.AddNumberImpl.addNumbers(AddNumberImpl.java:39) at org.apache.cxf.systest.ws.addr_wsdl.AddNumberImpl.execute(AddNumberImpl.java:53) at org.apache.cxf.systest.ws.addr_wsdl.AddNumberImpl.addNumbers(AddNumberImpl.java:39) at org.apache.cxf.systest.ws.addr_wsdl.jaxwsmm.WSDLAddrPolicyAttachmentJaxwsMMProviderTest.testUsingAddressing(WSDLAddrPolicyAttachmentJaxwsMMProviderTest.java:117) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/15984//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15984//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/15984//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15984//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/15984//console This message is automatically generated.
          Hide
          carp84 Yu Li added a comment -

          Latest patch taking usage of WeakObjectPool introduced by HBASE-14268. Since HBASE-14268 is committed, let's see what HadoopQA says with this patch

          Show
          carp84 Yu Li added a comment - Latest patch taking usage of WeakObjectPool introduced by HBASE-14268 . Since HBASE-14268 is committed, let's see what HadoopQA says with this patch
          Hide
          carp84 Yu Li added a comment -

          A summary of the recent back and forth discussion and changes:

          Each block in the bucket cache has an offset recording its position in the buckets, and it's used to get/evict the block from the cache. By design we couldn't evict a block in read, so we need a lock (in this JIRA we use read write lock to replace synchronize) as the gate-keeper for each offset, and such offset->lock pair (i.e. "entry") are stored in a hash map.

          To avoid garbages, efforts are paid to remove the entry from the map when it's our of usage, but after removing synchronized block the original patch introduced a lock leak issue. Patch v3~v5 tried to resolve the problem but either with performance issue or went in the wrong way.

          The latest patch in rb (not attached here since it depends on some non-committed code) resolved the lock leak problem by saving the remove action instead of dealing with synchronization on it. It uses WeakObjectPool introduced in HBASE-14268 which utilizes weak reference object to let system gc clear the non-used entries automatically, meantime making sure lock in use won't got removed (gc won't clear object with strong reference). This should be a complete solution to the only issue left here, and I think the patch is ready to go after HBASE-14268 got in

          Let me know if different thoughts, thanks.

          Show
          carp84 Yu Li added a comment - A summary of the recent back and forth discussion and changes: Each block in the bucket cache has an offset recording its position in the buckets, and it's used to get/evict the block from the cache. By design we couldn't evict a block in read, so we need a lock (in this JIRA we use read write lock to replace synchronize) as the gate-keeper for each offset, and such offset->lock pair (i.e. "entry") are stored in a hash map. To avoid garbages, efforts are paid to remove the entry from the map when it's our of usage, but after removing synchronized block the original patch introduced a lock leak issue. Patch v3~v5 tried to resolve the problem but either with performance issue or went in the wrong way. The latest patch in rb (not attached here since it depends on some non-committed code) resolved the lock leak problem by saving the remove action instead of dealing with synchronization on it. It uses WeakObjectPool introduced in HBASE-14268 which utilizes weak reference object to let system gc clear the non-used entries automatically, meantime making sure lock in use won't got removed (gc won't clear object with strong reference). This should be a complete solution to the only issue left here, and I think the patch is ready to go after HBASE-14268 got in Let me know if different thoughts, thanks.
          Hide
          ndimiduk Nick Dimiduk added a comment -

          Dropping released versions of 1.x for now, we can reassess when the sum of changes necessary are understood.

          Show
          ndimiduk Nick Dimiduk added a comment - Dropping released versions of 1.x for now, we can reassess when the sum of changes necessary are understood.
          Hide
          carp84 Yu Li added a comment -

          Mark this JIRA depending on HBASE-14268 since it needs the WeakObjectPool introduced there.

          Show
          carp84 Yu Li added a comment - Mark this JIRA depending on HBASE-14268 since it needs the WeakObjectPool introduced there.
          Hide
          carp84 Yu Li added a comment -

          Sorry, you are right, I thought it in the wrong way...

          I checked your implementation of WeakObjectPool and it should be able to resolve the problem here. I have been thinking of a way to remove the map w/o generating garbage but never thought of using weak reference. Nice work there!

          I checked the performance w/ WeakObjectPool and it looks good (~0.5s with blocksize=16k). Will update the patch in rb and mark this JIRA depending on HBASE-14268

          Show
          carp84 Yu Li added a comment - Sorry, you are right, I thought it in the wrong way... I checked your implementation of WeakObjectPool and it should be able to resolve the problem here. I have been thinking of a way to remove the map w/o generating garbage but never thought of using weak reference. Nice work there! I checked the performance w/ WeakObjectPool and it looks good (~0.5s with blocksize=16k). Will update the patch in rb and mark this JIRA depending on HBASE-14268
          Hide
          ikeda Hiroshi Ikeda added a comment -

          Sorry I still don't understand the relation between read/write locks of lockOnMap and "one write many read" block cache. It seems enough to cause contention around lockOnMap by many reading block cache.

          Show
          ikeda Hiroshi Ikeda added a comment - Sorry I still don't understand the relation between read/write locks of lockOnMap and "one write many read" block cache. It seems enough to cause contention around lockOnMap by many reading block cache.
          Hide
          carp84 Yu Li added a comment -

          Hi Hiroshi Ikeda,

          Yes I totally understand your concern, that one block eviction will cause blocking on reading another unrelated block. However, as I mentioned before, the blockcache should be a one write many read thing, or say much more hits than eviction (if not the case, I think it's recommended to disable blockcache to save the miss and load cost). So I think the cost is acceptable, and the test result with block-evicting thread could prove my statement.

          OTOH, I agree that we could do lock striping for further improvement, but per the above comment I think it's not that critical w/o such improvements, and I'd prefer to open another JIRA for it instead of blocking this one (Rome was not built in a day, right ). Makes sense?

          stack, Anoop Sam John, Ted Yu and Jingcheng Du, please also let me know your thoughts, thanks.

          Show
          carp84 Yu Li added a comment - Hi Hiroshi Ikeda , Yes I totally understand your concern, that one block eviction will cause blocking on reading another unrelated block. However, as I mentioned before, the blockcache should be a one write many read thing, or say much more hits than eviction (if not the case, I think it's recommended to disable blockcache to save the miss and load cost). So I think the cost is acceptable, and the test result with block-evicting thread could prove my statement. OTOH, I agree that we could do lock striping for further improvement, but per the above comment I think it's not that critical w/o such improvements, and I'd prefer to open another JIRA for it instead of blocking this one (Rome was not built in a day, right ). Makes sense? stack , Anoop Sam John , Ted Yu and Jingcheng Du , please also let me know your thoughts, thanks.
          Hide
          ikeda Hiroshi Ikeda added a comment -

          If you use non-fair mode, it is possible to unexpectedly long wait to lock a write lock when reading is congest so that there is almost always an unrelated thread waiting an unrelated read-lock. But fair mode might unexpectedly block everything and quite reduce performance.

          Lock striping or using some tricks for ConcurrentMap is also applicable, but in this case HBASE-14268 might help you, which uses weak references. If you prefer Google Guava, you can create your own class using Google Guava's CacheBuilder.

          Show
          ikeda Hiroshi Ikeda added a comment - If you use non-fair mode, it is possible to unexpectedly long wait to lock a write lock when reading is congest so that there is almost always an unrelated thread waiting an unrelated read-lock. But fair mode might unexpectedly block everything and quite reduce performance. Lock striping or using some tricks for ConcurrentMap is also applicable, but in this case HBASE-14268 might help you, which uses weak references. If you prefer Google Guava, you can create your own class using Google Guava's CacheBuilder.
          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/12762608/HBASE-14463_v5.patch
          against master branch at commit 526520de0a9d7a29fcf1b4c521f017ca75a46cbc.
          ATTACHMENT ID: 12762608

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

          +1 tests included. The patch appears to include 8 new or modified tests.

          +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.7.0 2.7.1)

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

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

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

          +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch.

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

          -1 core zombie tests. There are 1 zombie test(s): at org.apache.hadoop.mapred.TestFixedLengthInputFormat.testFormatCompressedIn(TestFixedLengthInputFormat.java:90)

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/15776//testReport/
          Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15776//artifact/patchprocess/newFindbugsWarnings.html
          Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/15776//artifact/patchprocess/checkstyle-aggregate.html

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/15776//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/12762608/HBASE-14463_v5.patch against master branch at commit 526520de0a9d7a29fcf1b4c521f017ca75a46cbc. ATTACHMENT ID: 12762608 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 8 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.client.TestShell org.apache.hadoop.hbase.client.TestReplicationShell -1 core zombie tests . There are 1 zombie test(s): at org.apache.hadoop.mapred.TestFixedLengthInputFormat.testFormatCompressedIn(TestFixedLengthInputFormat.java:90) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/15776//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15776//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/15776//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/15776//console This message is automatically generated.
          Hide
          carp84 Yu Li added a comment -

          Notice that in this UT case we are evicting a block every 10 milliseconds to demo the read/write contention, which is the reason of the change on perf number (was 0.318s before adding the block evict thread). But in real world there should be much less read/write contention since BucketCache uses LRU as its eviction algorithm. I've also confirmed on my local env that increasing the eviction period (like to 100ms) would bring a much better number (similar to 0.318s, with some deviation in different run)

          Show
          carp84 Yu Li added a comment - Notice that in this UT case we are evicting a block every 10 milliseconds to demo the read/write contention, which is the reason of the change on perf number (was 0.318s before adding the block evict thread). But in real world there should be much less read/write contention since BucketCache uses LRU as its eviction algorithm. I've also confirmed on my local env that increasing the eviction period (like to 100ms) would bring a much better number (similar to 0.318s, with some deviation in different run)
          Hide
          carp84 Yu Li added a comment -

          New patch adds an additional thread to evict the single-key block every 10ms in CacheTestUtils#hammerSingleKey to check the performance with read/write contention, and perf number before/after patch are as follows:

          w/ IdLock and blocksize=16k: 155.836s
          w/ IdReadWriteLock and blocksize=16k: 1.162s

          Refer to the attached junit screenshot for more details.

          Show
          carp84 Yu Li added a comment - New patch adds an additional thread to evict the single-key block every 10ms in CacheTestUtils#hammerSingleKey to check the performance with read/write contention, and perf number before/after patch are as follows: w/ IdLock and blocksize=16k: 155.836s w/ IdReadWriteLock and blocksize=16k: 1.162s Refer to the attached junit screenshot for more details.
          Hide
          carp84 Yu Li added a comment -

          Sure, I actually have one at hand since our online cluster is using 0.98. Will upload the patch when all discussions have a conclusion and patch looks good for everybody.

          Show
          carp84 Yu Li added a comment - Sure, I actually have one at hand since our online cluster is using 0.98. Will upload the patch when all discussions have a conclusion and patch looks good for everybody.
          Hide
          carp84 Yu Li added a comment -

          I assume the "lock" mentioned above means the "lockOnMap", right? Notice that it's the lock on the map recording entries for each block offset in the bucketcache, not the lock inside the entry, and we already use different ReentrantReadWriteLocks for different entries.

          Regarding the lockOnMap, yes you are right that read and write on the map will block each other. However, since cache is a "one write many read" thing, I think the read/write contention is limited. We could see this as a trade off: a little bit more cost when read/write on same block happens but lots of improvement for the parallel read which happens more frequently.

          The lockOnMap is necessary to prevent the lock leak issue you and Anoop mentioned previously. Actually I thought about removing the block-offset->entry map by maintaining an array of locks for each offset during the buckets instantiation, however, I found this design not that efficient since an initial 64k bucket might change to 128k bucket later if more 128k blocks in need. So I'm afraid we have to take the trade-off here.

          Performance of the exiting code is mentioned in my previous comments, so allow me to simply quote here:

          Time to run TestBucketCache#testCacheMultiThreadedSingleKey with the new implementation compared with old ones (also attached the JUnit screenshot):
          w/ IdLock with blocksize=16384: 19.676s
          w/ IdReadWriteLock-old with blocksize=16384: 0.172s
          w/ IdReadWriteLock-new with blocksize=16384: 0.318s

          The current TestBucketCache#testCacheMultiThreadedSingleKey case only includes read operation on single key, but after all the discussion here, I'd like to change it adding some write operations, and make sure write less than read.

          JingchengDu and Hiroshi Ikeda, feel free to let me know your thoughts.

          Show
          carp84 Yu Li added a comment - I assume the "lock" mentioned above means the "lockOnMap", right? Notice that it's the lock on the map recording entries for each block offset in the bucketcache, not the lock inside the entry, and we already use different ReentrantReadWriteLocks for different entries. Regarding the lockOnMap, yes you are right that read and write on the map will block each other. However, since cache is a "one write many read" thing, I think the read/write contention is limited. We could see this as a trade off: a little bit more cost when read/write on same block happens but lots of improvement for the parallel read which happens more frequently. The lockOnMap is necessary to prevent the lock leak issue you and Anoop mentioned previously. Actually I thought about removing the block-offset->entry map by maintaining an array of locks for each offset during the buckets instantiation, however, I found this design not that efficient since an initial 64k bucket might change to 128k bucket later if more 128k blocks in need. So I'm afraid we have to take the trade-off here. Performance of the exiting code is mentioned in my previous comments, so allow me to simply quote here: Time to run TestBucketCache#testCacheMultiThreadedSingleKey with the new implementation compared with old ones (also attached the JUnit screenshot): w/ IdLock with blocksize=16384: 19.676s w/ IdReadWriteLock-old with blocksize=16384: 0.172s w/ IdReadWriteLock-new with blocksize=16384: 0.318s The current TestBucketCache#testCacheMultiThreadedSingleKey case only includes read operation on single key, but after all the discussion here, I'd like to change it adding some write operations, and make sure write less than read. JingchengDu and Hiroshi Ikeda , feel free to let me know your thoughts.
          Hide
          apurtell Andrew Purtell added a comment -

          Bug/perf fix going back to 0.98, updated fix versions

          Show
          apurtell Andrew Purtell added a comment - Bug/perf fix going back to 0.98, updated fix versions
          Hide
          jingcheng.du@intel.com Jingcheng Du added a comment -

          Agree with Hiroshi Ikeda, the lock acquire in one thread can block the lock release of unrelated threads.
          Maybe we can use different ReentrantReadWriteLocks for different entries? Besides, what is the performance with this patch comparing with the existing code? Thanks!

          Show
          jingcheng.du@intel.com Jingcheng Du added a comment - Agree with Hiroshi Ikeda , the lock acquire in one thread can block the lock release of unrelated threads. Maybe we can use different ReentrantReadWriteLocks for different entries? Besides, what is the performance with this patch comparing with the existing code? Thanks!
          Hide
          ikeda Hiroshi Ikeda added a comment -

          Is it OK that releasing any lock is blocked while someone is blocked to get an unrelated lock?

          Show
          ikeda Hiroshi Ikeda added a comment - Is it OK that releasing any lock is blocked while someone is blocked to get an unrelated lock?
          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/12762205/HBASE-14463_v4.patch
          against master branch at commit 20ed465d07ba02c66d8f1af5746ee59dd446cb95.
          ATTACHMENT ID: 12762205

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

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.7.0 2.7.1)

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

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

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

          +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch.

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

          -1 core zombie tests. There are 2 zombie test(s):

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/15725//testReport/
          Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15725//artifact/patchprocess/newFindbugsWarnings.html
          Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/15725//artifact/patchprocess/checkstyle-aggregate.html

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/15725//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/12762205/HBASE-14463_v4.patch against master branch at commit 20ed465d07ba02c66d8f1af5746ee59dd446cb95. ATTACHMENT ID: 12762205 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 5 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: -1 core zombie tests . There are 2 zombie test(s): Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/15725//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15725//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/15725//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/15725//console This message is automatically generated.
          Hide
          carp84 Yu Li added a comment -

          v4 patch resolves checkstyle issue reported by HadoopQA

          Show
          carp84 Yu Li added a comment - v4 patch resolves checkstyle issue reported by HadoopQA
          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/12762158/HBASE-14463_v3.patch
          against master branch at commit 5b7894f92ba3e9ff700da1e9194ebb4774d8b71e.
          ATTACHMENT ID: 12762158

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

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.7.0 2.7.1)

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

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

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

          -1 checkstyle. The applied patch generated 1807 checkstyle errors (more than the master's current 1805 errors).

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch.

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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/15720//testReport/
          Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15720//artifact/patchprocess/newFindbugsWarnings.html
          Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/15720//artifact/patchprocess/checkstyle-aggregate.html

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/15720//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/12762158/HBASE-14463_v3.patch against master branch at commit 5b7894f92ba3e9ff700da1e9194ebb4774d8b71e. ATTACHMENT ID: 12762158 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 5 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. -1 checkstyle . The applied patch generated 1807 checkstyle errors (more than the master's current 1805 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/15720//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15720//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/15720//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/15720//console This message is automatically generated.
          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/12762157/HBASE-14463_v2.patch
          against master branch at commit 5b7894f92ba3e9ff700da1e9194ebb4774d8b71e.
          ATTACHMENT ID: 12762157

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

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.7.0 2.7.1)

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

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

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

          -1 checkstyle. The applied patch generated 1807 checkstyle errors (more than the master's current 1805 errors).

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch.

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

          -1 core zombie tests. There are 1 zombie test(s):

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/15719//testReport/
          Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15719//artifact/patchprocess/newFindbugsWarnings.html
          Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/15719//artifact/patchprocess/checkstyle-aggregate.html

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/15719//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/12762157/HBASE-14463_v2.patch against master branch at commit 5b7894f92ba3e9ff700da1e9194ebb4774d8b71e. ATTACHMENT ID: 12762157 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 5 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. -1 checkstyle . The applied patch generated 1807 checkstyle errors (more than the master's current 1805 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: -1 core zombie tests . There are 1 zombie test(s): Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/15719//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15719//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/15719//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/15719//console This message is automatically generated.
          Hide
          carp84 Yu Li added a comment -

          Upload v3 patch to resolve the lock leak issue pointed by JingchengDu and Anoop Sam John, in sync with rb

          Time to run TestBucketCache#testCacheMultiThreadedSingleKey with the new implementation compared with old ones (also attached the JUnit screenshot):

          w/ IdLock with blocksize=16384: 19.676s
          w/ IdReadWriteLock-old with blocksize=16384: 0.172s
          w/ IdReadWriteLock-new with blocksize=16384: 0.318s

          Show
          carp84 Yu Li added a comment - Upload v3 patch to resolve the lock leak issue pointed by JingchengDu and Anoop Sam John , in sync with rb Time to run TestBucketCache#testCacheMultiThreadedSingleKey with the new implementation compared with old ones (also attached the JUnit screenshot): w/ IdLock with blocksize=16384: 19.676s w/ IdReadWriteLock-old with blocksize=16384: 0.172s w/ IdReadWriteLock-new with blocksize=16384: 0.318s
          Hide
          carp84 Yu Li added a comment -

          Upload v2 patch in sync with rb, addressing several review comments

          Show
          carp84 Yu Li added a comment - Upload v2 patch in sync with rb, addressing several review comments
          Hide
          carp84 Yu Li added a comment -

          Nice catch! Thanks for point this issue out Jingcheng Du

          And thanks Anoop Sam John for the detailed explanation in rb.

          Let me think carefully about how to resolve this issue, I tried some methods but still haven't figured out a good way. Kind of understanding the original while loop there in IdLock now...

          Show
          carp84 Yu Li added a comment - Nice catch! Thanks for point this issue out Jingcheng Du And thanks Anoop Sam John for the detailed explanation in rb. Let me think carefully about how to resolve this issue, I tried some methods but still haven't figured out a good way. Kind of understanding the original while loop there in IdLock now...
          Hide
          anoop.hbase Anoop Sam John added a comment -

          Ya am agreeing with Jingcheng. I dont think we can remove synchronized fully this way.

          Show
          anoop.hbase Anoop Sam John added a comment - Ya am agreeing with Jingcheng. I dont think we can remove synchronized fully this way.
          Hide
          jingcheng.du@intel.com Jingcheng Du added a comment -

          Thanks Yu Li!
          We have two places to use IdLock in mob, MobFileCache and HMobStore, where IdLock is used as a write lock. If the performance of IdReadWriteLock can be improved in write mode, I think you can use IdReadWriteLock in mob as well.
          In MobFileCache, the evict is not to evict blocks from the cache, we just evict the un-referenced file reader from the cache. It's ok to evict when reading.
          Besides, you remove the loop in getLockEntry, and remove sync from both getLockEntry and releaseLockEntry, what if a race condition in these methods of IdReadWriteLock, a thread acquires a write lock but it is removed from the map by another thread because of a race condition(the code

          entry.readWriteLock.hasQueuedThreads()

          and

          boolean removeSucceed = map.remove(entry.id, entry)

          in releaseLockEntry give the race condition a chance). It is possible, right?

          Show
          jingcheng.du@intel.com Jingcheng Du added a comment - Thanks Yu Li ! We have two places to use IdLock in mob, MobFileCache and HMobStore, where IdLock is used as a write lock. If the performance of IdReadWriteLock can be improved in write mode, I think you can use IdReadWriteLock in mob as well. In MobFileCache, the evict is not to evict blocks from the cache, we just evict the un-referenced file reader from the cache. It's ok to evict when reading. Besides, you remove the loop in getLockEntry, and remove sync from both getLockEntry and releaseLockEntry, what if a race condition in these methods of IdReadWriteLock, a thread acquires a write lock but it is removed from the map by another thread because of a race condition(the code entry.readWriteLock.hasQueuedThreads() and boolean removeSucceed = map.remove(entry.id, entry) in releaseLockEntry give the race condition a chance). It is possible, right?
          Hide
          carp84 Yu Li added a comment -

          Ok, got your point now. And yes, will check the incr/append performance when got the patch and UT done. Will also check whether could get some data from online, thanks for the reminder.

          Show
          carp84 Yu Li added a comment - Ok, got your point now. And yes, will check the incr/append performance when got the patch and UT done. Will also check whether could get some data from online, thanks for the reminder.
          Hide
          vrodionov Vladimir Rodionov added a comment -

          Sorry but could you further clarify your thought Vladimir Rodionov

          The same IdLock is used in HFileReaderImpl.readBlock. We lock before accessing block cache. Multiple threads competing for block access on a read path is usual for incr/append, I presume. If counters are global.

          Show
          vrodionov Vladimir Rodionov added a comment - Sorry but could you further clarify your thought Vladimir Rodionov The same IdLock is used in HFileReaderImpl.readBlock. We lock before accessing block cache. Multiple threads competing for block access on a read path is usual for incr/append, I presume. If counters are global.
          Hide
          carp84 Yu Li added a comment -

          Sorry but could you further clarify your thought Vladimir Rodionov? I didn't quite catch you about the inc/append part. The issue here exists in BlockCache(BucketCache) and there shouldn't be any inc/append involved based on my understanding

          Regarding the 100X perf enhancement, I didn't see as much enhancement in 0.98, but that's the testing result for trunk on my env. I tried 5 times for each case(IdLock/IdReadWriteLock) with TestBucketCache and got very similar result, so should be no occasional. In theory for read it's like serial v.s. parallel on race condition, so I think the enhancement is kind of expected.

          Show
          carp84 Yu Li added a comment - Sorry but could you further clarify your thought Vladimir Rodionov ? I didn't quite catch you about the inc/append part. The issue here exists in BlockCache(BucketCache) and there shouldn't be any inc/append involved based on my understanding Regarding the 100X perf enhancement, I didn't see as much enhancement in 0.98, but that's the testing result for trunk on my env. I tried 5 times for each case(IdLock/IdReadWriteLock) with TestBucketCache and got very similar result, so should be no occasional. In theory for read it's like serial v.s. parallel on race condition, so I think the enhancement is kind of expected.
          Hide
          vrodionov Vladimir Rodionov added a comment -

          No. it is different, Stack, HBASE-14460 is about mvcc contention, this one is about IdLock contention. Both should be fixed.

          Show
          vrodionov Vladimir Rodionov added a comment - No. it is different, Stack , HBASE-14460 is about mvcc contention, this one is about IdLock contention. Both should be fixed.
          Hide
          stack stack added a comment -

          ...that explains incr/append degradation as well.

          You talking HBASE-14460 here Vladimir Rodionov ? Thanks.

          Show
          stack stack added a comment - ...that explains incr/append degradation as well. You talking HBASE-14460 here Vladimir Rodionov ? Thanks.
          Hide
          carp84 Yu Li added a comment -

          Thanks for the information about HBASE-13903 Matteo Bertozzi.

          Yes, we almost focused on the same code segment, and the phenomenon is similar too. The only difference is that in our case I saw lots of handler waiting for the entry lock to be released(existing.wait) instead of map.putIfAbsent. Below is the jstack I got while encountering the online issue:

          "B.DefaultRpcServer.handler=127,queue=10,port=60020" daemon prio=10 tid=0x00007f7556bda800 nid=0x123b1 in Object.wait() [0x00000000449ae000]
             java.lang.Thread.State: WAITING (on object monitor)
          	at java.lang.Object.wait(Native Method)
          	at java.lang.Object.wait(Object.java:503)
          	at org.apache.hadoop.hbase.util.IdLock.getLockEntry(IdLock.java:79)
          	- locked <0x000000017e2e0980> (a org.apache.hadoop.hbase.util.IdLock$Entry)
          	at org.apache.hadoop.hbase.io.hfile.bucket.BucketCache.getBlock(BucketCache.java:413)
          	at org.apache.hadoop.hbase.io.hfile.CombinedBlockCache.getBlock(CombinedBlockCache.java:77)
          	at org.apache.hadoop.hbase.io.hfile.HFileReaderV2.readBlock(HFileReaderV2.java:360)
          

          And this is the relative code:

          73    while ((existing = map.putIfAbsent(entry.id, entry)) != null) {
          74      synchronized (existing) {
          75        if (existing.isLocked) {
          76          ++existing.numWaiters;  // Add ourselves to waiters.
          77          while (existing.isLocked) {
          78            try {
          79              existing.wait();
          80            } catch (InterruptedException e) {
          

          Regarding move IdLock to be a write-only readwrite lock, I thought about using IdReadWriteLock to fully replace IdLock but saw reference from MobFileCache besides BucketCache. I guess it could also benefit since file open/close cost is also expensive but not that sure since I never tried MobFileCache in real env. Also I'm not sure whether we shouldn't evict the block while still having threads reading it, if so I guess we still need read+write lock. Jingcheng Du could you give some comments here about the MobFileCache case?

          Show
          carp84 Yu Li added a comment - Thanks for the information about HBASE-13903 Matteo Bertozzi . Yes, we almost focused on the same code segment, and the phenomenon is similar too. The only difference is that in our case I saw lots of handler waiting for the entry lock to be released(existing.wait) instead of map.putIfAbsent. Below is the jstack I got while encountering the online issue: "B.DefaultRpcServer.handler=127,queue=10,port=60020" daemon prio=10 tid=0x00007f7556bda800 nid=0x123b1 in Object.wait() [0x00000000449ae000] java.lang.Thread.State: WAITING (on object monitor) at java.lang.Object.wait(Native Method) at java.lang.Object.wait(Object.java:503) at org.apache.hadoop.hbase.util.IdLock.getLockEntry(IdLock.java:79) - locked <0x000000017e2e0980> (a org.apache.hadoop.hbase.util.IdLock$Entry) at org.apache.hadoop.hbase.io.hfile.bucket.BucketCache.getBlock(BucketCache.java:413) at org.apache.hadoop.hbase.io.hfile.CombinedBlockCache.getBlock(CombinedBlockCache.java:77) at org.apache.hadoop.hbase.io.hfile.HFileReaderV2.readBlock(HFileReaderV2.java:360) And this is the relative code: 73 while ((existing = map.putIfAbsent(entry.id, entry)) != null ) { 74 synchronized (existing) { 75 if (existing.isLocked) { 76 ++existing.numWaiters; // Add ourselves to waiters. 77 while (existing.isLocked) { 78 try { 79 existing.wait(); 80 } catch (InterruptedException e) { Regarding move IdLock to be a write-only readwrite lock, I thought about using IdReadWriteLock to fully replace IdLock but saw reference from MobFileCache besides BucketCache. I guess it could also benefit since file open/close cost is also expensive but not that sure since I never tried MobFileCache in real env. Also I'm not sure whether we shouldn't evict the block while still having threads reading it, if so I guess we still need read+write lock. Jingcheng Du could you give some comments here about the MobFileCache case?
          Hide
          vrodionov Vladimir Rodionov added a comment -

          Matteo Bertozzi, yes, that explains incr/append degradation as well. Yu Li, can you test increment performance as well, multithreaded?

          Show
          vrodionov Vladimir Rodionov added a comment - Matteo Bertozzi , yes, that explains incr/append degradation as well. Yu Li , can you test increment performance as well, multithreaded?
          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/12761651/HBASE-14463.patch
          against master branch at commit 45d67435adc9195325bfccc78b7e1a0202a446e5.
          ATTACHMENT ID: 12761651

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

          +1 tests included. The patch appears to include 5 new or modified tests.

          +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.7.0 2.7.1)

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

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

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

          -1 checkstyle. The applied patch generated 1826 checkstyle errors (more than the master's current 1823 errors).

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch.

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

          -1 core zombie tests. There are 1 zombie test(s): at org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.TestNodeLabelContainerAllocation.testContainerAllocateWithComplexLabels(TestNodeLabelContainerAllocation.java:293)

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/15674//testReport/
          Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15674//artifact/patchprocess/newFindbugsWarnings.html
          Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/15674//artifact/patchprocess/checkstyle-aggregate.html

          Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15674//artifact/patchprocess/patchJavadocWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/15674//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/12761651/HBASE-14463.patch against master branch at commit 45d67435adc9195325bfccc78b7e1a0202a446e5. ATTACHMENT ID: 12761651 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 5 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. -1 javadoc . The javadoc tool appears to have generated 7 warning messages. -1 checkstyle . The applied patch generated 1826 checkstyle errors (more than the master's current 1823 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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 post-site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.client.TestReplicationShell -1 core zombie tests . There are 1 zombie test(s): at org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.TestNodeLabelContainerAllocation.testContainerAllocateWithComplexLabels(TestNodeLabelContainerAllocation.java:293) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/15674//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15674//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/15674//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/15674//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/15674//console This message is automatically generated.
          Hide
          mbertozzi Matteo Bertozzi added a comment -

          from what I have seen in HBASE-13903 the main problems was the while() loop ending up with too many calls to map.putIfAbsent()/synchronized to handle the "entry removed" case

          Show
          mbertozzi Matteo Bertozzi added a comment - from what I have seen in HBASE-13903 the main problems was the while() loop ending up with too many calls to map.putIfAbsent()/synchronized to handle the "entry removed" case
          Hide
          vrodionov Vladimir Rodionov added a comment -

          You replaced synchronized access with ReentrantReadWriteLock and got 100x perf improvement? Am I missing something else?

          Show
          vrodionov Vladimir Rodionov added a comment - You replaced synchronized access with ReentrantReadWriteLock and got 100x perf improvement? Am I missing something else?
          Hide
          mbertozzi Matteo Bertozzi added a comment -

          in HBASE-13903 I had a simple perf test for the IdLock.
          I did a try, and on my run looks like the ReadWrite lock has perf close to the patch proposed in HBASE-13903 even when used as "write only" lock.
          https://gist.github.com/matteobertozzi/b3ff0ce79eaac8ece446
          so, in my opinion we should also move IdLock to be a Write-only ReadWrite lock. (if others can confirm the perf results)

          Show
          mbertozzi Matteo Bertozzi added a comment - in HBASE-13903 I had a simple perf test for the IdLock. I did a try, and on my run looks like the ReadWrite lock has perf close to the patch proposed in HBASE-13903 even when used as "write only" lock. https://gist.github.com/matteobertozzi/b3ff0ce79eaac8ece446 so, in my opinion we should also move IdLock to be a Write-only ReadWrite lock. (if others can confirm the perf results)
          Hide
          carp84 Yu Li added a comment -

          rb link here: https://reviews.apache.org/r/38626

          Will submit the patch to ask HadoopQA to check first.

          Show
          carp84 Yu Li added a comment - rb link here: https://reviews.apache.org/r/38626 Will submit the patch to ask HadoopQA to check first.
          Hide
          carp84 Yu Li added a comment -

          Actually the performance issue also shows in our UT case, say TestBucketCache#testCacheMultiThreadedSingleKey. Times to run this case w/ and w/o patch are as follows:

          w/ IdReadWriteLock and blocksize=16384: 0.172s
          w/ IdLock and blocksize=16384: 19.676s

          Also attach the screenshots of JUnit result

          Show
          carp84 Yu Li added a comment - Actually the performance issue also shows in our UT case, say TestBucketCache#testCacheMultiThreadedSingleKey. Times to run this case w/ and w/o patch are as follows: w/ IdReadWriteLock and blocksize=16384: 0.172s w/ IdLock and blocksize=16384: 19.676s Also attach the screenshots of JUnit result
          Hide
          carp84 Yu Li added a comment -

          Attaching the initial patch

          Show
          carp84 Yu Li added a comment - Attaching the initial patch

            People

            • Assignee:
              carp84 Yu Li
              Reporter:
              carp84 Yu Li
            • Votes:
              0 Vote for this issue
              Watchers:
              16 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development