[improve][broker] Avoid O(n) pending ack scans for size lookup#26019
Open
void-ptr974 wants to merge 5 commits into
Open
[improve][broker] Avoid O(n) pending ack scans for size lookup#26019void-ptr974 wants to merge 5 commits into
void-ptr974 wants to merge 5 commits into
Conversation
Member
In practice a single consumer won't be handling very many ledgers at once so this isn't a bottleneck or major waste of CPU. Regardless, this is a useful improvement. |
dao-jun
approved these changes
Jun 13, 2026
merlimat
approved these changes
Jun 13, 2026
| try { | ||
| readLock.lock(); | ||
| return pendingAcks.values().stream().mapToInt(TreeMap::size).sum(); | ||
| return size; |
Contributor
There was a problem hiding this comment.
We could remove the read lock here if we make the size as volatile (and still update it while holding the lock).
Contributor
Author
There was a problem hiding this comment.
Done, thanks for the suggestion. This makes size() even cheaper.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Motivation
PendingAcksMap.size()currently computes the pending ack count by scanning all per-ledger pending ack maps. This makes a simple size lookup O(number of ledgers), even though callers only need the total number of pending ack entries.On consumers with many pending acknowledgments, this adds unnecessary CPU cost to paths that inspect or drain pending acks.
Modifications
Maintain a cached pending ack count in
PendingAcksMap.The counter is updated when pending ack entries are added, removed, range-removed, or cleared. With this,
PendingAcksMap.size()becomes an O(1) read instead of scanning nested maps.The change keeps the existing
TreeMapstructure and only optimizes size accounting.Correctness
PendingAcksMapTestverifies size accounting across these paths:updateRemainingUnackedremoveAndGet, including not-foundremoveAllUpTowithin one ledger and across multiple ledgersforEachAndClearandforEachAndClosePerformance
Simple local size lookup microbenchmark on OpenJDK 25.0.2. The benchmark compares the previous map-scan size calculation with the cached counter read, both under a
ReentrantReadWriteLockread lock.The exact numbers are environment-dependent, but the result shows the intended change in complexity: size lookup no longer grows with the number of tracked ledgers.
Verifying this change
./gradlew :pulsar-broker:test --tests org.apache.pulsar.broker.service.PendingAcksMapTest./gradlew :pulsar-broker:checkstyleMain