[improve][common] Optimize TopicName.get() to reduce lock contention on cache lookup#25367
Conversation
…on cache lookup ### Motivation `TopicName.get()` previously used `ConcurrentHashMap.computeIfAbsent()` to populate the topic-name cache. Although `computeIfAbsent` is atomic, it holds the internal bin-lock for the entire duration of the mapping function, which includes the non-trivial `TopicName` construction (string splitting, validation, etc.). Under high-concurrency workloads where many threads simultaneously encounter the same uncached topic name, this causes unnecessary lock contention and can degrade throughput. ### Modifications Replace `computeIfAbsent` with an explicit two-step pattern: 1. **Fast path**: call `cache.get(topic)` first — a single volatile read with no locking — and return immediately on a cache hit (steady-state case). 2. **Slow path** (cache miss): construct `TopicName` *outside* the lock, then use `cache.putIfAbsent()` to insert. If two threads race on the same key, one wins the `putIfAbsent` and the other's instance is discarded; this is safe because `TopicName` is immutable. Add a Javadoc comment on `get()` explaining the rationale.
Background While load-testing Pulsar with a single topic containing 1,000,000 partitions, we observed that Root Cause Analysis Two compounding issues were identified:
Fix Replace // Before
return cache.computeIfAbsent(topic, TopicName::new);
// After
TopicName cached = cache.get(topic);
if (cached != null) return cached;
TopicName created = new TopicName(topic); // constructed outside lock
TopicName existing = cache.putIfAbsent(topic, created);
return existing != null ? existing : created;Under concurrent misses on the same key, each thread constructs its own instance independently; Benchmark Results (JMH · SingleShotTime · 8 threads · 1M partitions · full cold cache)
The tail latency improvement (1.75× at p99) is especially significant: 
|
merlimat
left a comment
There was a problem hiding this comment.
Change looks good.
can you try to compare to just call cache .put() instead of putIfAbsent()?
It would be good if you could add the jhm code to the microbenchmarks module
|
Related PR from the past #24457 (change was rejected so I closed it) with a lot of interesting comments. |
lhotari
left a comment
There was a problem hiding this comment.
This doesn't address one of the key problems which is that the cache is cleared if the size exceeds the max size setting (topicNameCacheMaxCapacity):
We'd be better off by switching to use Caffeine again. The Caffeine author commented in #24457 (comment) that the bottleneck in the earlier version has been addressed. Since we have upgraded to Java 17 for the client, we can use the newer Caffeine version.
|
The flamegraph in #25367 (comment) looks like it was from an old version of Pulsar that uses Guava Cache (before #23052 <3.0.6, <3.3.1). |
The benchmark seems to measure the miss of 1M cache entries. That's not a very realistic scenario that there would be such amount of misses at once. I think it would be more useful to have a solution that ensures that the memory use of the cache is bounded. in #24868 The max size is limited to 20% of max heapsize. For TopicName cache there could be a bytesize limit too. The StringInterner solution would be useful at least for namespace and tenant Strings to ensure that those don't cause heap duplication if the namespace or tenant caches expire or overflow. |
The benchmark doesn't use actual code which would be part of the Pulsar code base. |
The flamegraph comes from 3.0.5, I change version to 3.0.13 and make
In my scenario, one topic has 1 million partitions, so there are 1 million cache misses during the initial startup. Topic loading is currently very slow, and I suspect this may be one of the reasons. |
Thanks for pointing out the historical context and Ben Manes' insights! Just to clarify the scope of this PR: the That said, I'd like to confirm whether this small optimization is still worthwhile on its own:
So the question is: do you see any reason this low-risk change should be blocked on the larger cache redesign? Happy to fold it into a bigger PR if that's preferred, but wanted to check if it can stand alone first. |
lhotari
left a comment
There was a problem hiding this comment.
LGTM, great solution @liangyepianzhou. Just one comment about documenting implementation details in javadoc.
Thanks for the suggestion. Fixed. |
|
I think we should not depend much from the cache itself. The |
@BewareMyPower Could you rebase #24463? I think we can handle that optimization separately. On the caching: one benefit worth preserving is that the TopicName and NamespaceName caches reduce java.lang.String instance duplication, which is valuable beyond just avoiding the construction cost itself. So I'd lean toward keeping the caches rather than removing them. |
@BewareMyPower After applying your optimization, the benchmark results are as follows: jmh2 uses Benchmark Mode Cnt Score Error Units Benchmark Mode Cnt Score Error Units |
|
so the result is now with this PR (replace
|
|
I'm rebasing my PR currently. But it's a bit confusing from your test result. It seems that switching to |
Are you testing the cold start / cache-miss scenario? My understanding is that the main latency comes from the cache-miss startup phase, where the optimization is most noticeable. You can use my JMH program to test it. |
|
No. I didn't run any test for now. I'm just analyzing your test report here: #25367 (comment)
Oh my bad, I misunderstood the unit ( |
…on cache lookup (#25367) ### Motivation `TopicName.get()` previously used `ConcurrentHashMap.computeIfAbsent()` to populate the topic-name cache. Although `computeIfAbsent` is atomic, it holds the internal bin-lock for the entire duration of the mapping function, which includes the non-trivial `TopicName` construction (string splitting, validation, etc.). Under high-concurrency workloads where many threads simultaneously encounter the same uncached topic name, this causes unnecessary lock contention and can degrade throughput. ### Modifications Replace `computeIfAbsent` with an explicit two-step pattern: 1. **Fast path**: call `cache.get(topic)` first — a single volatile read with no locking — and return immediately on a cache hit (steady-state case). 2. **Slow path** (cache miss): construct `TopicName` *outside* the lock, then use `cache.put()` to insert. (cherry picked from commit 8c4e83d)
…on cache lookup (#25367) ### Motivation `TopicName.get()` previously used `ConcurrentHashMap.computeIfAbsent()` to populate the topic-name cache. Although `computeIfAbsent` is atomic, it holds the internal bin-lock for the entire duration of the mapping function, which includes the non-trivial `TopicName` construction (string splitting, validation, etc.). Under high-concurrency workloads where many threads simultaneously encounter the same uncached topic name, this causes unnecessary lock contention and can degrade throughput. ### Modifications Replace `computeIfAbsent` with an explicit two-step pattern: 1. **Fast path**: call `cache.get(topic)` first — a single volatile read with no locking — and return immediately on a cache hit (steady-state case). 2. **Slow path** (cache miss): construct `TopicName` *outside* the lock, then use `cache.put()` to insert. (cherry picked from commit 8c4e83d)
…on cache lookup (#25367) ### Motivation `TopicName.get()` previously used `ConcurrentHashMap.computeIfAbsent()` to populate the topic-name cache. Although `computeIfAbsent` is atomic, it holds the internal bin-lock for the entire duration of the mapping function, which includes the non-trivial `TopicName` construction (string splitting, validation, etc.). Under high-concurrency workloads where many threads simultaneously encounter the same uncached topic name, this causes unnecessary lock contention and can degrade throughput. ### Modifications Replace `computeIfAbsent` with an explicit two-step pattern: 1. **Fast path**: call `cache.get(topic)` first — a single volatile read with no locking — and return immediately on a cache hit (steady-state case). 2. **Slow path** (cache miss): construct `TopicName` *outside* the lock, then use `cache.put()` to insert. (cherry picked from commit 8c4e83d)
…on cache lookup (#25367) ### Motivation `TopicName.get()` previously used `ConcurrentHashMap.computeIfAbsent()` to populate the topic-name cache. Although `computeIfAbsent` is atomic, it holds the internal bin-lock for the entire duration of the mapping function, which includes the non-trivial `TopicName` construction (string splitting, validation, etc.). Under high-concurrency workloads where many threads simultaneously encounter the same uncached topic name, this causes unnecessary lock contention and can degrade throughput. ### Modifications Replace `computeIfAbsent` with an explicit two-step pattern: 1. **Fast path**: call `cache.get(topic)` first — a single volatile read with no locking — and return immediately on a cache hit (steady-state case). 2. **Slow path** (cache miss): construct `TopicName` *outside* the lock, then use `cache.put()` to insert. (cherry picked from commit 8c4e83d)
…on cache lookup (apache#25367) ### Motivation `TopicName.get()` previously used `ConcurrentHashMap.computeIfAbsent()` to populate the topic-name cache. Although `computeIfAbsent` is atomic, it holds the internal bin-lock for the entire duration of the mapping function, which includes the non-trivial `TopicName` construction (string splitting, validation, etc.). Under high-concurrency workloads where many threads simultaneously encounter the same uncached topic name, this causes unnecessary lock contention and can degrade throughput. ### Modifications Replace `computeIfAbsent` with an explicit two-step pattern: 1. **Fast path**: call `cache.get(topic)` first — a single volatile read with no locking — and return immediately on a cache hit (steady-state case). 2. **Slow path** (cache miss): construct `TopicName` *outside* the lock, then use `cache.put()` to insert. (cherry picked from commit 8c4e83d) (cherry picked from commit 069d1a4)
Motivation
TopicName.get()previously usedConcurrentHashMap.computeIfAbsent()to populate the topic-name cache. AlthoughcomputeIfAbsentis atomic, it holds the internal bin-lock for the entire duration of the mapping function, which includes the non-trivialTopicNameconstruction (string splitting, validation, etc.).Under high-concurrency workloads where many threads simultaneously encounter the same uncached topic name, this causes unnecessary lock contention and can degrade throughput.
Modifications
Replace
computeIfAbsentwith an explicit two-step pattern:cache.get(topic)first — a single volatile read with no locking — and return immediately on a cache hit (steady-state case).TopicNameoutside the lock, then usecache.putIfAbsent()to insert. If two threads race on the same key, one wins theputIfAbsentand the other's instance is discarded; this is safe becauseTopicNameis immutable.Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: