liangyepianzhou commented on PR #25367: URL: https://github.com/apache/pulsar/pull/25367#issuecomment-4234579497
> The flamegraph in [#25367 (comment)](https://github.com/apache/pulsar/pull/25367#issuecomment-4095828178) looks like it was from an old version of Pulsar that uses Guava Cache (before #23052 <3.0.6, <3.3.1). @liangyepianzhou What version of Pulsar are you load testing? Thanks for pointing out the historical context and Ben Manes' insights! Just to clarify the scope of this PR: the `get + putIfAbsent` change is intentionally a minimal, targeted fix to reduce bin-lock contention during cache misses, independent of the broader cache design questions (capacity bounding, Caffeine version, soft references, etc.). That said, I'd like to confirm whether this small optimization is still worthwhile on its own: 1. Even with a properly bounded cache, concurrent cache misses (e.g. during a cold start or after a cache clear) will still happen. In those cases, `putIfAbsent` avoids holding the bin-lock during `TopicName` construction, which seems beneficial regardless of the eviction strategy. 2. The fast path (`cache.get()` first) is a strict improvement for the steady-state hit case with zero synchronization overhead. 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
