[Partition Consumer] Provide lock-free access to compression providers #689
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
dferstay
force-pushed
the
lock-free-consumer-compression-providers
branch
from
9bf63d9
to
96bf631
Compare
dferstay
changed the title
wolfstudy
reviewed
Dec 24, 2021
pulsar/consumer_partition.go
Outdated
| pc.providersMutex.RLock() | ||
| provider, ok := pc.compressionProviders[msgMeta.GetCompression()] | ||
| pc.providersMutex.RUnlock() | ||
| v, ok := pc.compressionProviders.Load(msgMeta.GetCompression()) |
There was a problem hiding this comment.
Maybe we can replace v with meaningful variable names
There was a problem hiding this comment.
@wolfstudy , I've renamed the variables in this method for additional clarity.
The compression providers map in the partition consumer is a textbook case for using go's lock-free sync.Map: the set of map entries is stable and access is read-only. On machines with 4 cores or greater, read contention on the sync.RWMutex outweighs the cost of using a sync.Map. Below is an old article on the subject, but it still holds true today: https://medium.com/@deckarep/the-new-kid-in-town-gos-sync-map-de24a6bf7c2c Signed-off-by: Daniel Ferstay <dferstay@splunk.com>
dferstay
force-pushed
the
lock-free-consumer-compression-providers
branch
from
0a8e77c
to
ac0a159
Compare
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.
Motivation
The compression providers map in the partition consumer is a textbook case
for using go's lock-free sync.Map: the set of map entries is stable and
access is read-only.
On machines with 4 cores or greater, read contention on the sync.RWMutex
outweighs the cost of using a sync.Map.
Below is an old article on the subject, but it still holds true today:
https://medium.com/@deckarep/the-new-kid-in-town-gos-sync-map-de24a6bf7c2c
Modifications
The sync.RWMutex and map of compression providers (
map[pb.CompressionType]compression.Provider) in the partition consumer has been replaced with a sync.MapVerifying this change
This change is already covered by existing tests, such as:
pulsar/consumer_partitiion_test.goDoes this pull request potentially affect one of the following parts:
Documentation