Improved efficiency in DigestManager.verify() #3810
Merged
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## master #3810 +/- ##
============================================
- Coverage 60.47% 60.41% -0.07%
+ Complexity 5847 5844 -3
============================================
Files 473 473
Lines 40933 40937 +4
Branches 5235 5234 -1
============================================
- Hits 24756 24731 -25
- Misses 13976 13991 +15
- Partials 2201 2215 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Please rebase the master @merlimat |
merlimat
force-pushed
the
digest-manager
branch
from
526d640 to
855d68d
Compare
hangc0276
approved these changes
Feb 27, 2023
| private static final FastThreadLocal<ByteBuf> DIGEST_BUFFER = new FastThreadLocal<ByteBuf>() { | ||
| @Override | ||
| protected ByteBuf initialValue() throws Exception { | ||
| return PooledByteBufAllocator.DEFAULT.directBuffer(1024); |
There was a problem hiding this comment.
The required ByteBuf size for each digest manager:
CRC32CDigestManager -> 4
CRC32DigestManager -> 8
DummyDigestManager -> 0
MacDigestManager -> 20
Do we need to allocate 1024 bytes?
There was a problem hiding this comment.
Just in case. Though this is only used for MAC at this point, and one per thread. It shouldn't be any noticeable waste.
There was a problem hiding this comment.
I actually was referring to my next change in the DigestManager :)
hangc0276
pushed a commit
that referenced
this pull request
Feb 27, 2023
### Motivation In `DigestManager` there are several accesses to ThreadLocal variable per each entry processed. The reason is the mainly due to `DigestManager` API which exposes a stateful `update()` method which can be invoked multiple times and keeps the current checksum as a thread-local variable. If we exclude MAC digest which is 20 bytes, for other digests we can instead keep the current checksum in a local variable and pass it each time, avoiding all the thread-locals and also the need for writing the checksum result into a buffer. ### Benchmarks #### Before #3810 ``` Benchmark (entrySize) Mode Cnt Score Error Units DigestManagerBenchmark.verifyDigest 64 thrpt 3 13.450 ± 3.634 ops/us DigestManagerBenchmark.verifyDigest 1024 thrpt 3 7.908 ± 2.637 ops/us DigestManagerBenchmark.verifyDigest 4086 thrpt 3 3.233 ± 0.882 ops/us DigestManagerBenchmark.verifyDigest 8192 thrpt 3 1.846 ± 0.047 ops/us ``` #### After #3810 ``` Benchmark (entrySize) Mode Cnt Score Error Units DigestManagerBenchmark.verifyDigest 64 thrpt 3 46.312 ± 7.414 ops/us DigestManagerBenchmark.verifyDigest 1024 thrpt 3 13.379 ± 1.069 ops/us DigestManagerBenchmark.verifyDigest 4086 thrpt 3 3.787 ± 0.059 ops/us DigestManagerBenchmark.verifyDigest 8192 thrpt 3 1.956 ± 0.052 ops/us ``` #### After this change ``` Benchmark (entrySize) Mode Cnt Score Error Units DigestManagerBenchmark.verifyDigest 64 thrpt 3 130.108 ± 4.854 ops/us DigestManagerBenchmark.verifyDigest 1024 thrpt 3 17.744 ± 0.238 ops/us DigestManagerBenchmark.verifyDigest 4086 thrpt 3 4.104 ± 0.181 ops/us DigestManagerBenchmark.verifyDigest 8192 thrpt 3 2.050 ± 0.066 ops/us ```
hezhangjian
pushed a commit
that referenced
this pull request
Mar 1, 2023
### Motivation In #3810 the signature of `Crc32cIntChecksum.resumeChecksum()` was changed to accept `offset` & `len` in the buffer. Since this method is also used externally (in Pulsar), we should leave also the old method signature to avoid breaking the API when upgrading BK.
Ghatage
pushed a commit
to sijie/bookkeeper
that referenced
this pull request
Jul 12, 2024
Ghatage
pushed a commit
to sijie/bookkeeper
that referenced
this pull request
Jul 12, 2024
### Motivation In `DigestManager` there are several accesses to ThreadLocal variable per each entry processed. The reason is the mainly due to `DigestManager` API which exposes a stateful `update()` method which can be invoked multiple times and keeps the current checksum as a thread-local variable. If we exclude MAC digest which is 20 bytes, for other digests we can instead keep the current checksum in a local variable and pass it each time, avoiding all the thread-locals and also the need for writing the checksum result into a buffer. ### Benchmarks #### Before apache#3810 ``` Benchmark (entrySize) Mode Cnt Score Error Units DigestManagerBenchmark.verifyDigest 64 thrpt 3 13.450 ± 3.634 ops/us DigestManagerBenchmark.verifyDigest 1024 thrpt 3 7.908 ± 2.637 ops/us DigestManagerBenchmark.verifyDigest 4086 thrpt 3 3.233 ± 0.882 ops/us DigestManagerBenchmark.verifyDigest 8192 thrpt 3 1.846 ± 0.047 ops/us ``` #### After apache#3810 ``` Benchmark (entrySize) Mode Cnt Score Error Units DigestManagerBenchmark.verifyDigest 64 thrpt 3 46.312 ± 7.414 ops/us DigestManagerBenchmark.verifyDigest 1024 thrpt 3 13.379 ± 1.069 ops/us DigestManagerBenchmark.verifyDigest 4086 thrpt 3 3.787 ± 0.059 ops/us DigestManagerBenchmark.verifyDigest 8192 thrpt 3 1.956 ± 0.052 ops/us ``` #### After this change ``` Benchmark (entrySize) Mode Cnt Score Error Units DigestManagerBenchmark.verifyDigest 64 thrpt 3 130.108 ± 4.854 ops/us DigestManagerBenchmark.verifyDigest 1024 thrpt 3 17.744 ± 0.238 ops/us DigestManagerBenchmark.verifyDigest 4086 thrpt 3 4.104 ± 0.181 ops/us DigestManagerBenchmark.verifyDigest 8192 thrpt 3 2.050 ± 0.066 ops/us ```
Ghatage
pushed a commit
to sijie/bookkeeper
that referenced
this pull request
Jul 12, 2024
### Motivation In apache#3810 the signature of `Crc32cIntChecksum.resumeChecksum()` was changed to accept `offset` & `len` in the buffer. Since this method is also used externally (in Pulsar), we should leave also the old method signature to avoid breaking the API when upgrading BK.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
In
DigestManager.verifyDigestAndReturnData()there are a couple of simple improvements:Benchmarks:
Before
Allocations: 230 bytes per operation
After
Allocations: 64 bytes per operation
Note: these remaining 64 bytes are only due to the Java9 reflection access. It would not be present on the JNI-based CRC32c backend (though that does not work on Mac M1).