You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2021/06/21 11:12:54 UTC
[GitHub] [hbase] virajjasani opened a new pull request #3407: HBASE-26018. Perf improvement in L1 cache
virajjasani opened a new pull request #3407:
URL: https://github.com/apache/hbase/pull/3407
* CHM#computeIfPresent takes lock on bucket but CHM#get is lockless
* Atomically retaining refCount is coming up bit expensive in terms of performance issues
* When we see aggressive cache hits for meta blocks (with major blocks in cache), we would want to get away from coarse grained locking
* Treat cache read API as optimistic read
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] saintstack commented on a change in pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#discussion_r686972375
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruAdaptiveBlockCache.java
##########
@@ -646,14 +639,16 @@ private long updateSizeMetrics(LruCachedBlock cb, boolean evict) {
@Override
public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat,
boolean updateCacheMetrics) {
- LruCachedBlock cb = map.computeIfPresent(cacheKey, (key, val) -> {
- // It will be referenced by RPC path, so increase here. NOTICE: Must do the retain inside
- // this block. because if retain outside the map#computeIfPresent, the evictBlock may remove
- // the block and release, then we're retaining a block with refCnt=0 which is disallowed.
- // see HBASE-22422.
- val.getBuffer().retain();
- return val;
- });
+ LruCachedBlock cb = map.get(cacheKey);
+ if (cb != null) {
+ try {
+ cb.getBuffer().retain();
Review comment:
(Sorry, forgot to submit my comment from a good while ago)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#discussion_r666482218
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruAdaptiveBlockCache.java
##########
@@ -646,14 +639,16 @@ private long updateSizeMetrics(LruCachedBlock cb, boolean evict) {
@Override
public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat,
boolean updateCacheMetrics) {
- LruCachedBlock cb = map.computeIfPresent(cacheKey, (key, val) -> {
- // It will be referenced by RPC path, so increase here. NOTICE: Must do the retain inside
- // this block. because if retain outside the map#computeIfPresent, the evictBlock may remove
- // the block and release, then we're retaining a block with refCnt=0 which is disallowed.
- // see HBASE-22422.
- val.getBuffer().retain();
- return val;
- });
+ LruCachedBlock cb = map.get(cacheKey);
+ if (cb != null) {
+ try {
+ cb.getBuffer().retain();
Review comment:
I think for possibility#2 in above, we stand a chance where buffer with non-zero refCount is not in the cache. I see, let me see what alternatives we have for this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on pull request #3407: HBASE-26018. Perf improvement in L1 cache
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-864949628
FYI @ben-manes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani closed pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
virajjasani closed pull request #3407:
URL: https://github.com/apache/hbase/pull/3407
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
apurtell commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-882784422
> Atomically retaining refCount is coming up bit expensive in terms of performance
@virajjasani In what context? What test case? What does "bit expensive" mean? Can we see the data?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] saintstack commented on a change in pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#discussion_r671532481
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruAdaptiveBlockCache.java
##########
@@ -646,14 +639,16 @@ private long updateSizeMetrics(LruCachedBlock cb, boolean evict) {
@Override
public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat,
boolean updateCacheMetrics) {
- LruCachedBlock cb = map.computeIfPresent(cacheKey, (key, val) -> {
- // It will be referenced by RPC path, so increase here. NOTICE: Must do the retain inside
- // this block. because if retain outside the map#computeIfPresent, the evictBlock may remove
- // the block and release, then we're retaining a block with refCnt=0 which is disallowed.
- // see HBASE-22422.
- val.getBuffer().retain();
- return val;
- });
+ LruCachedBlock cb = map.get(cacheKey);
+ if (cb != null) {
+ try {
+ cb.getBuffer().retain();
Review comment:
Looking at this more.... I don't think we can do your trick afterall.
The refcounting is not for the cache, it is for a backing pool of memory used reading data in from hdfs into the cache. When we evict a block from the cache, we call #release on the memory. If the refcount is zero, the memory is released and can be reused in the backing pool. If #release is called and the #refcount is not zero, we just decrement the refcount.
A cached buffer item detached from the cache still needs to have its #release called w/ refcount at zero so the backing memory gets readded to the pool.
So it seems to me. What you think @virajjasani
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3407: HBASE-26018. Perf improvement in L1 cache
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-865238770
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 30s | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 9s | master passed |
| +1 :green_heart: | compile | 1m 1s | master passed |
| +1 :green_heart: | shadedjars | 8m 10s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 39s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 51s | the patch passed |
| +1 :green_heart: | compile | 1m 0s | the patch passed |
| +1 :green_heart: | javac | 1m 0s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 9s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 38s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 150m 18s | hbase-server in the patch failed. |
| | | 180m 40s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/2/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3407 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 32bd5afd5cdd 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9f4177f7b4 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/2/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/2/testReport/ |
| Max. process+thread count | 4244 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/2/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on pull request #3407: HBASE-26018. Perf improvement in L1 cache
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-864949628
FYI @ben-manes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#discussion_r666785190
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruAdaptiveBlockCache.java
##########
@@ -646,14 +639,16 @@ private long updateSizeMetrics(LruCachedBlock cb, boolean evict) {
@Override
public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat,
boolean updateCacheMetrics) {
- LruCachedBlock cb = map.computeIfPresent(cacheKey, (key, val) -> {
- // It will be referenced by RPC path, so increase here. NOTICE: Must do the retain inside
- // this block. because if retain outside the map#computeIfPresent, the evictBlock may remove
- // the block and release, then we're retaining a block with refCnt=0 which is disallowed.
- // see HBASE-22422.
- val.getBuffer().retain();
- return val;
- });
+ LruCachedBlock cb = map.get(cacheKey);
+ if (cb != null) {
+ try {
+ cb.getBuffer().retain();
Review comment:
Thanks @saintstack.
> After, biggest locking consumer was elsewhere and much more insignificant percentage
Does this mean we can kind of ignore this case (assuming objects not in cache will get GC'ed regardless of their netty based refCount)? Still thinking about this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#discussion_r687501045
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruAdaptiveBlockCache.java
##########
@@ -646,14 +639,16 @@ private long updateSizeMetrics(LruCachedBlock cb, boolean evict) {
@Override
public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat,
boolean updateCacheMetrics) {
- LruCachedBlock cb = map.computeIfPresent(cacheKey, (key, val) -> {
- // It will be referenced by RPC path, so increase here. NOTICE: Must do the retain inside
- // this block. because if retain outside the map#computeIfPresent, the evictBlock may remove
- // the block and release, then we're retaining a block with refCnt=0 which is disallowed.
- // see HBASE-22422.
- val.getBuffer().retain();
- return val;
- });
+ LruCachedBlock cb = map.get(cacheKey);
+ if (cb != null) {
+ try {
+ cb.getBuffer().retain();
Review comment:
> A cached buffer item detached from the cache still needs to have its #release called w/ refcount at zero so the backing memory gets readded to the pool.
Yeah I think this makes sense. Let me get back to this in case I find some better and obvious way to improve perf and get some YCSB results.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3407: HBASE-26018. Perf improvement in L1 cache
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-865349287
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 32s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 23s | master passed |
| +1 :green_heart: | compile | 1m 13s | master passed |
| +1 :green_heart: | shadedjars | 8m 24s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 42s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 20s | the patch passed |
| +1 :green_heart: | compile | 1m 16s | the patch passed |
| +1 :green_heart: | javac | 1m 16s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 40s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 53s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 140m 55s | hbase-server in the patch passed. |
| | | 173m 19s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/3/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3407 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 4cb5fda308d8 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / c5461aaa5b |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/3/testReport/ |
| Max. process+thread count | 4182 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/3/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
apurtell commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-882784422
> Atomically retaining refCount is coming up bit expensive in terms of performance
@virajjasani In what context? What test case? What does "bit expensive" mean? Can we see the data?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3407: HBASE-26018. Perf improvement in L1 cache
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-865236347
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 30s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 47s | master passed |
| +1 :green_heart: | compile | 1m 13s | master passed |
| +1 :green_heart: | shadedjars | 8m 23s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 44s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 25s | the patch passed |
| +1 :green_heart: | compile | 1m 14s | the patch passed |
| +1 :green_heart: | javac | 1m 14s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 34s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 50s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 144m 38s | hbase-server in the patch failed. |
| | | 177m 19s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3407 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 4569d87b06f9 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9f4177f7b4 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/2/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/2/testReport/ |
| Max. process+thread count | 4141 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/2/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3407: HBASE-26018. Perf improvement in L1 cache
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-864984371
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 39s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 5m 5s | master passed |
| +1 :green_heart: | compile | 3m 43s | master passed |
| +1 :green_heart: | checkstyle | 1m 15s | master passed |
| +1 :green_heart: | spotbugs | 2m 16s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 21s | the patch passed |
| +1 :green_heart: | compile | 3m 33s | the patch passed |
| +1 :green_heart: | javac | 3m 33s | the patch passed |
| -0 :warning: | checkstyle | 1m 11s | hbase-server: The patch generated 2 new + 10 unchanged - 0 fixed = 12 total (was 10) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 23m 57s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0. |
| +1 :green_heart: | spotbugs | 2m 32s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 17s | The patch does not generate ASF License warnings. |
| | | 59m 56s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3407 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile |
| uname | Linux b71799d12e78 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9f4177f7b4 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 86 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/1/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3407: HBASE-26018. Perf improvement in L1 cache
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-865353410
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 32s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 43s | master passed |
| +1 :green_heart: | compile | 1m 0s | master passed |
| +1 :green_heart: | shadedjars | 8m 13s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 38s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 47s | the patch passed |
| +1 :green_heart: | compile | 1m 1s | the patch passed |
| +1 :green_heart: | javac | 1m 1s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 12s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 36s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 149m 30s | hbase-server in the patch passed. |
| | | 179m 27s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/3/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3407 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 3dd6e57e0847 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / c5461aaa5b |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/3/testReport/ |
| Max. process+thread count | 4165 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/3/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] saintstack commented on a change in pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#discussion_r666432156
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruAdaptiveBlockCache.java
##########
@@ -646,14 +639,16 @@ private long updateSizeMetrics(LruCachedBlock cb, boolean evict) {
@Override
public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat,
boolean updateCacheMetrics) {
- LruCachedBlock cb = map.computeIfPresent(cacheKey, (key, val) -> {
- // It will be referenced by RPC path, so increase here. NOTICE: Must do the retain inside
- // this block. because if retain outside the map#computeIfPresent, the evictBlock may remove
- // the block and release, then we're retaining a block with refCnt=0 which is disallowed.
- // see HBASE-22422.
- val.getBuffer().retain();
- return val;
- });
+ LruCachedBlock cb = map.get(cacheKey);
+ if (cb != null) {
+ try {
+ cb.getBuffer().retain();
Review comment:
And if it is purge from cache by a background thread, we'll have a cb w/ a non-zero refcount that is not in the cache? Will that work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3407: HBASE-26018. Perf improvement in L1 cache
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-865151240
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 7s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 17s | master passed |
| +1 :green_heart: | compile | 3m 24s | master passed |
| +1 :green_heart: | checkstyle | 1m 10s | master passed |
| +1 :green_heart: | spotbugs | 2m 13s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 0s | the patch passed |
| +1 :green_heart: | compile | 3m 21s | the patch passed |
| +1 :green_heart: | javac | 3m 21s | the patch passed |
| -0 :warning: | checkstyle | 1m 6s | hbase-server: The patch generated 2 new + 10 unchanged - 0 fixed = 12 total (was 10) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 20m 3s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0. |
| +1 :green_heart: | spotbugs | 2m 20s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 13s | The patch does not generate ASF License warnings. |
| | | 51m 33s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3407 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile |
| uname | Linux f3e07c467113 4.15.0-128-generic #131-Ubuntu SMP Wed Dec 9 06:57:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9f4177f7b4 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/2/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 86 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/2/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] saintstack commented on a change in pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#discussion_r666514850
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruAdaptiveBlockCache.java
##########
@@ -646,14 +639,16 @@ private long updateSizeMetrics(LruCachedBlock cb, boolean evict) {
@Override
public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat,
boolean updateCacheMetrics) {
- LruCachedBlock cb = map.computeIfPresent(cacheKey, (key, val) -> {
- // It will be referenced by RPC path, so increase here. NOTICE: Must do the retain inside
- // this block. because if retain outside the map#computeIfPresent, the evictBlock may remove
- // the block and release, then we're retaining a block with refCnt=0 which is disallowed.
- // see HBASE-22422.
- val.getBuffer().retain();
- return val;
- });
+ LruCachedBlock cb = map.get(cacheKey);
+ if (cb != null) {
+ try {
+ cb.getBuffer().retain();
Review comment:
#1 sounds good.
#2 yeah, it can get interesting. The computeIfPresent made reasoning easier for sure.
Running w/ #get instead of #computeIfPresent -- even though it incorrect -- changed the locking profile of a loaded process; before the change, the blockage in computeIfPresent was the biggest blockage. After, biggest locking consumer was elsewhere and much more insignificant percentage
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] saintstack commented on a change in pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#discussion_r668917910
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruAdaptiveBlockCache.java
##########
@@ -646,14 +639,16 @@ private long updateSizeMetrics(LruCachedBlock cb, boolean evict) {
@Override
public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat,
boolean updateCacheMetrics) {
- LruCachedBlock cb = map.computeIfPresent(cacheKey, (key, val) -> {
- // It will be referenced by RPC path, so increase here. NOTICE: Must do the retain inside
- // this block. because if retain outside the map#computeIfPresent, the evictBlock may remove
- // the block and release, then we're retaining a block with refCnt=0 which is disallowed.
- // see HBASE-22422.
- val.getBuffer().retain();
- return val;
- });
+ LruCachedBlock cb = map.get(cacheKey);
+ if (cb != null) {
+ try {
+ cb.getBuffer().retain();
Review comment:
@virajjasani Thats an interesting idea. Whether onheap or offheap, if no references -- i.e. not tied to a pool -- then they should get GC'd. Does the CB get returned to the cache when done?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3407: HBASE-26018. Perf improvement in L1 cache
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-865279962
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 5s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 58s | master passed |
| +1 :green_heart: | compile | 3m 21s | master passed |
| +1 :green_heart: | checkstyle | 1m 9s | master passed |
| +1 :green_heart: | spotbugs | 2m 10s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 3s | the patch passed |
| +1 :green_heart: | compile | 3m 19s | the patch passed |
| +1 :green_heart: | javac | 3m 19s | the patch passed |
| -0 :warning: | checkstyle | 1m 8s | hbase-server: The patch generated 2 new + 10 unchanged - 0 fixed = 12 total (was 10) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 20m 0s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0. |
| +1 :green_heart: | spotbugs | 2m 20s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 11s | The patch does not generate ASF License warnings. |
| | | 51m 6s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3407 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile |
| uname | Linux a94f555b7b5a 4.15.0-128-generic #131-Ubuntu SMP Wed Dec 9 06:57:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / c5461aaa5b |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/3/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 86 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/3/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
apurtell commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-882784422
> Atomically retaining refCount is coming up bit expensive in terms of performance
@virajjasani In what context? What test case? What does "bit expensive" mean? Can we see the data?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3407: HBASE-26018. Perf improvement in L1 cache
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-864984371
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3407: HBASE-26018. Perf improvement in L1 cache
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#issuecomment-865067631
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 33s | Docker mode activated. |
| -0 :warning: | yetus | 0m 5s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 11s | master passed |
| +1 :green_heart: | compile | 1m 3s | master passed |
| +1 :green_heart: | shadedjars | 8m 6s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 36s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 48s | the patch passed |
| +1 :green_heart: | compile | 1m 0s | the patch passed |
| +1 :green_heart: | javac | 1m 0s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 4s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 36s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 150m 26s | hbase-server in the patch failed. |
| | | 180m 41s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3407 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 10ea9c6a4187 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9f4177f7b4 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/1/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/1/testReport/ |
| Max. process+thread count | 4181 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3407/1/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#discussion_r671717888
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruAdaptiveBlockCache.java
##########
@@ -646,14 +639,16 @@ private long updateSizeMetrics(LruCachedBlock cb, boolean evict) {
@Override
public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat,
boolean updateCacheMetrics) {
- LruCachedBlock cb = map.computeIfPresent(cacheKey, (key, val) -> {
- // It will be referenced by RPC path, so increase here. NOTICE: Must do the retain inside
- // this block. because if retain outside the map#computeIfPresent, the evictBlock may remove
- // the block and release, then we're retaining a block with refCnt=0 which is disallowed.
- // see HBASE-22422.
- val.getBuffer().retain();
- return val;
- });
+ LruCachedBlock cb = map.get(cacheKey);
+ if (cb != null) {
+ try {
+ cb.getBuffer().retain();
Review comment:
> Does the CB get returned to the cache when done?
You mean if CB gets returned to L1 cache (CHM) after it's buffer has served read request? Yes, that's the case (unless I misunderstood the question)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#discussion_r666473427
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruAdaptiveBlockCache.java
##########
@@ -646,14 +639,16 @@ private long updateSizeMetrics(LruCachedBlock cb, boolean evict) {
@Override
public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat,
boolean updateCacheMetrics) {
- LruCachedBlock cb = map.computeIfPresent(cacheKey, (key, val) -> {
- // It will be referenced by RPC path, so increase here. NOTICE: Must do the retain inside
- // this block. because if retain outside the map#computeIfPresent, the evictBlock may remove
- // the block and release, then we're retaining a block with refCnt=0 which is disallowed.
- // see HBASE-22422.
- val.getBuffer().retain();
- return val;
- });
+ LruCachedBlock cb = map.get(cacheKey);
+ if (cb != null) {
+ try {
+ cb.getBuffer().retain();
Review comment:
It is purged from cache by:
```
protected long evictBlock(LruCachedBlock block, boolean evictedByEvictionProcess) {
LruCachedBlock previous = map.remove(block.getCacheKey()); =======> removed from map
if (previous == null) {
return 0;
}
updateSizeMetrics(block, true);
long val = elements.decrementAndGet();
if (LOG.isTraceEnabled()) {
long size = map.size();
assertCounterSanity(size, val);
}
if (block.getBuffer().getBlockType().isData()) {
dataBlockElements.decrement();
}
if (evictedByEvictionProcess) {
// When the eviction of the block happened because of invalidation of HFiles, no need to
// update the stats counter.
stats.evicted(block.getCachedTime(), block.getCacheKey().isPrimary());
if (victimHandler != null) {
victimHandler.cacheBlock(block.getCacheKey(), block.getBuffer());
}
}
// Decrease the block's reference count, and if refCount is 0, then it'll auto-deallocate. DO
// NOT move this up because if do that then the victimHandler may access the buffer with
// refCnt = 0 which is disallowed.
previous.getBuffer().release(); ============================> buffer released
return block.heapSize();
}
```
Based on above mentioned eviction code, we have below mentioned possibilities when eviction and getBlock happens for the same block at the same time:
1. getBlock retrieves block from map, eviction removes it from map, eviction does release(), getBlock does retain() and encounters IllegalRefCount Exception, we handler it with this patch and treat it as cache miss.
2. getBlock retrieves block from map, eviction removes it from map, getBlock does retain(), eviction does release(). Since getBlock retain() was successful, it proceeds as successful cache hit, which happens even today with computeIfPresent. Subsequent getBlock call will return null as block was evicted previously.
3. eviction removes from map, getBlock gets null, it's clear cache miss.
I think we seem good here. WDYT @saintstack?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #3407: HBASE-26018 Perf improvement in L1 cache - Optimistic call to buffer.retain()
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #3407:
URL: https://github.com/apache/hbase/pull/3407#discussion_r666482218
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruAdaptiveBlockCache.java
##########
@@ -646,14 +639,16 @@ private long updateSizeMetrics(LruCachedBlock cb, boolean evict) {
@Override
public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat,
boolean updateCacheMetrics) {
- LruCachedBlock cb = map.computeIfPresent(cacheKey, (key, val) -> {
- // It will be referenced by RPC path, so increase here. NOTICE: Must do the retain inside
- // this block. because if retain outside the map#computeIfPresent, the evictBlock may remove
- // the block and release, then we're retaining a block with refCnt=0 which is disallowed.
- // see HBASE-22422.
- val.getBuffer().retain();
- return val;
- });
+ LruCachedBlock cb = map.get(cacheKey);
+ if (cb != null) {
+ try {
+ cb.getBuffer().retain();
Review comment:
I think for possibility#2 in above, we stand a chance where buffer with non-zero refCount is not in the cache. I see, let me see what alternatives we have for this case.
Although I still think that same case can happen even today.
getBlock does retain() which will bring refCount of BB to 2, while getBlock is busy updating stats, eviction thread can evict block from cache and it does release() which will bring refCount of BB to 1. So even in this case, we can positive refCount buffer which is evicted from cache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org