GH-3879: Add cache to optimize header match performance. #3934
Conversation
Fixes: spring-projects#3879 Issue link: spring-projects#3879 What Add a LRU cache for pattern match of KafkaHeaderMapper. Why? To improve CPU usage used by pattern match of KafkaHeaderMapper. Commonly, many Kafka records in the same topic will have the same header name. Currently, Pattern Match has O(M*N) time complexity, where M is pattern length, N is String length. If results of patterns match are cached and KafkaHeaderMapper uses it, KafkaHeaderMapper can expect improvement in terms of CPU usage. Signed-off-by: Sanghyeok An <[email protected]>
Signed-off-by: Sanghyeok An <[email protected]>
chickenchickenlove
changed the title
| /** | ||
| * A Cache that remembers whether a header name matches the multi-value pattern. | ||
| */ | ||
| class HeaderPatternMatchCache { |
There was a problem hiding this comment.
This is cool solution!
No days without learning something new.
Thank you! 😄
So, this has to private static to optimize memory consumption.
And all the method not public.
Signed-off-by: Sanghyeok An <[email protected]>
|
|
||
| private final ConcurrentLruCache<String, Boolean> multiValueHeaderPatternMatchCache = new ConcurrentLruCache<>(MAX_SIZE, key -> Boolean.TRUE); | ||
|
|
||
| private final ConcurrentLruCache<String, Boolean> singleValueHeaderPatternMatchCache = new ConcurrentLruCache<>(MAX_SIZE, key -> Boolean.TRUE); |
There was a problem hiding this comment.
Thanks for the update .
Now I have a question: why no logic like “if not multi-value , than it is single”?
I just said something here about memory , and realized that we do waste it with this map for single-value headers. While it feels like just worry about caching multi is enough.
Looks like we need to dedicate more engineering to this algorithm to come up with better utilization.
Does it make sense what I’m asking?
Thanks.
There was a problem hiding this comment.
The current implementation is designed to cache the result (either single-value or multi-value) for any header name that has undergone pattern matching at least once.
If we only store the result for multi-value headers, then even for header names that were already evaluated as single-value, the pattern matching logic would still be executed repeatedly each time.
Initially, I intended to use a single ConcurrentLruCache to store false for single-value and true for multi-value headers, but that behavior wasn’t supported🥲, which is why the current structure was chosen.
IMHO, for an effective pattern match cache, both outcomes—single-value and multi-value—should be stored. Currently, each ConcurrentLruCache is configured with a size of 1000. What do you think about reducing it to 500 to optimize memory usage?
Right, my bad. The real problem that we always go against pattern matching when we could just cache the matching outcome to not go there any more if we got the same header name in the next record. So, it sounds like we got a 3 states: Does this makes sense? |
|
@artembilan
// 1.
if (this.headerMatchedCache.isMultiValuePattern(headerName)) {
return true;
}
// 2.
if (this.headerMatchedCache.isSingleValuePattern(headerName)) {
return false;
}
// In this step, it is 'dont map' state.
// In case the pattern matches multi-value
// 3.
this.headerMatchedCache.cacheAsMultiValueHeader(headerName);
...
// In case the pattern doesn't match multi-value
// 4.
this.headerMatchedCache.cacheAsSingleValueHeader(headerName);
// After this, it is not in 'dont map' state.The Also, This is the reason why I used two separate IMHO, the race condition that might occur in What do you think? |
Fixes: #3879
Issue link: #3879
In previous discussion, we decided to use
LinkedHashMapforLruCacheImplementation.However, I think that we missed concurrency of KafkaListenerContainer.
We can set concurrency when we create KafkaListener like
@KafkaListener(concurrency = 10, ....).But,
LinkedHashMapis not thread-safe.So, I use
ConcurrentLruCacheprovided byspring-frameworkto ensure thread-safe.