Zstd encoder limit and raise encoder cache size #2979
Conversation
This file contains 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
A recent issue reported by Henry Haiying Cai showed that the current zstd reuse logic has 2 major flaws - There is no upper bound on created zstd encoders - The reuse of encoders is low if many goroutines hit `zstdCompress` simultaniously This is fixed by changing the original behavior in 2 ways - There are never more than GOMAXPROCs encoders in use - The maximum number of encoders per compression level is GOMAXPRCOS at some early point in time This means we have finally an upper bound on in-use encoders and with that a worst case memory consumption. Caching that amount of encoders does not worsen the worst case behavior (unless many compression levels are in use at the same time). This should be a significant performance improvement for codebases that generate many messages in parallel, fanning out to many partitions. Signed-off-by: René Treffer <treffer@measite.de>
rtreffer
force-pushed
the
zstd-encoder-limit
branch
from
a77069f
to
8e68941
Compare
Signed-off-by: René Treffer <treffer@measite.de>
rtreffer
force-pushed
the
zstd-encoder-limit
branch
from
b92a93f
to
a3e620a
Compare
| defer zstdMutex.Unlock() | ||
|
|
||
| limit := runtime.GOMAXPROCS(0) | ||
| for zstdCheckedOutEncoders >= limit && !zstdTestingDisableConcurrencyLimit { |
There was a problem hiding this comment.
- Is
zstdCheckedOutEncodersand the capacity of of zstdEncoderChannel are doing the same kind of control? - Is seems hard to follow if the
limitvalue from line 49 are different from line 52 or line 29, might need some comment or explanation.
There was a problem hiding this comment.
Some process could have changed the GOMAXPROCS in between calls. The proper behavior after calling a sync.Semaphore.Wait() is to refresh all values before performing any tests.
This gives a nice speedup at high compresison levels with no changes for message sizes at or below 1MB (the default). Signed-off-by: René Treffer <treffer@measite.de>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR addresses #2965 with a different approach.
This PR addresses 2 issues with the current implementation
The PR preserves the following property
The memory behavior of applications can change slightly.
Before applying the patch:
After applying the patch:
This should not change the worst case for the great majority of users, but it might be relevant in cases where applications were alternating between high sarama use and other uses.
There are 2 new benchmarks and a testing flag (zstdTestingDisableConcurrencyLimit) to verify the concurrency limiting.
I've also added some more information to the tests (like setting the bytes so throughput can be measured).
Here is a sample output from my machine (AMD framework 13):