Skip to content

Conversation

@merlimat
Copy link
Contributor

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

@merlimat merlimat added this to the 4.16.0 milestone Feb 27, 2023
@merlimat merlimat self-assigned this Feb 27, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2023

Codecov Report

Merging #3811 (2f77281) into master (1f8de8f) will increase coverage by 1.37%.
The diff coverage is 65.21%.

@@             Coverage Diff              @@
##             master    #3811      +/-   ##
============================================
+ Coverage     56.31%   57.69%   +1.37%     
- Complexity     5524     5774     +250     
============================================
  Files           473      473              
  Lines         40952    40963      +11     
  Branches       5235     5240       +5     
============================================
+ Hits          23064    23635     +571     
+ Misses        15790    15211     -579     
- Partials       2098     2117      +19     
Flag Coverage Δ
bookie 39.88% <47.82%> (?)
client 44.26% <65.21%> (-0.03%) ⬇️
replication ?
tls 21.03% <21.73%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...pache/bookkeeper/proto/checksum/DigestManager.java 67.52% <55.55%> (-6.27%) ⬇️
...bookkeeper/proto/checksum/CRC32CDigestManager.java 100.00% <100.00%> (ø)
.../bookkeeper/proto/checksum/CRC32DigestManager.java 83.33% <100.00%> (+1.51%) ⬆️
.../bookkeeper/proto/checksum/DummyDigestManager.java 100.00% <100.00%> (ø)
...he/bookkeeper/proto/checksum/MacDigestManager.java 82.14% <100.00%> (+0.66%) ⬆️
...rg/apache/bookkeeper/metastore/MetastoreTable.java 0.00% <0.00%> (-100.00%) ⬇️
.../bookkeeper/metastore/MetastoreScannableTable.java 0.00% <0.00%> (-100.00%) ⬇️
...bookkeeper/server/http/service/MetricsService.java 0.00% <0.00%> (-100.00%) ⬇️
...eper/server/http/service/ConfigurationService.java 0.00% <0.00%> (-100.00%) ⬇️
...per/meta/LongHierarchicalLedgerManagerFactory.java 0.00% <0.00%> (-100.00%) ⬇️
... and 252 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hangc0276 hangc0276 merged commit 1e02853 into apache:master Feb 27, 2023
@merlimat merlimat deleted the digest-no-thread-locals branch February 27, 2023 19:01
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
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants