HBASE-29218: Reduce calls to Configuration#get() in decompression path #6857
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… to Configuration.get()
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
charlesconnell
force-pushed
the
HBASE-29218/hfile-decompressing-context
branch
2 times, most recently
from
d424ae6 to
e8f9f96
Compare
charlesconnell
force-pushed
the
HBASE-29218/hfile-decompressing-context
branch
from
e8f9f96 to
99f82e4
Compare
Contributor
Author
|
I've added a commit to switch uses of the Caffeine cache back to Guava, in modules that leak into the client. I should have considered that using an un-shaded Caffeine would pollute client classpaths. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ndimiduk
approved these changes
Apr 17, 2025
Member
ndimiduk
left a comment
ndimiduk
left a comment
There was a problem hiding this comment.
Some questions / nits. Otherwise looks good to me.
| <groupId>org.apache.commons</groupId> | ||
| <artifactId>commons-crypto</artifactId> | ||
| </dependency> | ||
| <dependency> |
hbase-common/src/main/java/org/apache/hadoop/hbase/io/compress/DictionaryCache.java
Show resolved
Hide resolved
...mmon/src/main/java/org/apache/hadoop/hbase/io/encoding/HFileBlockDefaultDecodingContext.java
Show resolved
Hide resolved
| public static final String ZSTD_DICTIONARY_KEY = "hbase.io.compress.zstd.dictionary"; | ||
|
|
||
| private static final Cache<String, Pair<ZstdDictDecompress, Integer>> DECOMPRESS_DICT_CACHE = | ||
| CacheBuilder.newBuilder().maximumSize(100).expireAfterAccess(10, TimeUnit.MINUTES).build(); |
Member
There was a problem hiding this comment.
Should the details of this cache be user-configurable?
Contributor
Author
There was a problem hiding this comment.
My preference is for less low-value configuration options. But, I'm happy to do it either way, what do you prefer?
Member
There was a problem hiding this comment.
Nope, I'm fine with leaving them as hard-coded for now. Just asking the question to prompt the discussion.
...td/src/main/java/org/apache/hadoop/hbase/io/compress/zstd/ZstdHFileDecompressionContext.java
Outdated
Show resolved
Hide resolved
Member
|
Oh sorry. Please also address the checkstyle nits. Thanks @charlesconnell ! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ndimiduk
changed the title
ndimiduk
pushed a commit
to ndimiduk/hbase
that referenced
this pull request
Apr 24, 2025
apache#6857) Signed-off-by: Nick Dimiduk <[email protected]>
ndimiduk
pushed a commit
to ndimiduk/hbase
that referenced
this pull request
Apr 24, 2025
apache#6857) Signed-off-by: Nick Dimiduk <[email protected]>
ndimiduk
pushed a commit
that referenced
this pull request
Apr 24, 2025
#6857) Signed-off-by: Nick Dimiduk <[email protected]>
ndimiduk
added a commit
that referenced
this pull request
Apr 25, 2025
#6857) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Charles Connell <[email protected]>
ndimiduk
added a commit
to ndimiduk/hbase
that referenced
this pull request
Apr 25, 2025
apache#6857) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Charles Connell <[email protected]>
ndimiduk
added a commit
to ndimiduk/hbase
that referenced
this pull request
Apr 25, 2025
apache#6857) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Charles Connell <[email protected]>
ndimiduk
added a commit
that referenced
this pull request
Apr 26, 2025
#6857) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Charles Connell <[email protected]>
ndimiduk
added a commit
that referenced
this pull request
Apr 26, 2025
#6857) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Charles Connell <[email protected]>
mokai87
pushed a commit
to mokai87/hbase
that referenced
this pull request
Aug 7, 2025
apache#6857) Signed-off-by: Nick Dimiduk <[email protected]> Co-authored-by: Charles Connell <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use of the
org.apache.hadoop.conf.Configurationclass to look up values is not super fast. It's fine most of the time, but in a very hot code path, it takes up noticeable CPU time.ByteBuffDecompressor's are pooled and reused to avoid garbage collection churn. This means that sometimes their settings are not right for the block they're being asked to decompress. To handle this, before every decompression action, we callByteBuffDecompressor#reinit(Configuration), so it can pull settings from the Configuration in preparation for the decompression it's about to do. TheConfiguration#get()insidereinit()happens once per block, even though the settings it deals with are consistent across an entire table.Because the settings used by a
ByteBuffDecompressordon't actually change within a table, we can pull the settings it needs from aConfigurationwhen opening the HFile, and then not check again.