Ruby: Refactor the object cache to better account for race conditions#13075
Closed
casperisfine wants to merge 1 commit intoprotocolbuffers:mainfrom
Closed
Ruby: Refactor the object cache to better account for race conditions#13075casperisfine wants to merge 1 commit intoprotocolbuffers:mainfrom
casperisfine wants to merge 1 commit intoprotocolbuffers:mainfrom
Conversation
casperisfine
force-pushed
the
ruby-object-cache
branch
from
e127b5b to
bd19a22
Compare
fowles
reviewed
Jun 15, 2023
casperisfine
force-pushed
the
ruby-object-cache
branch
2 times, most recently
from
74dd06f to
bcd2cfd
Compare
fowles
approved these changes
Jun 16, 2023
Member
|
How do we validate that there is no performance regression? I think the relevant benchmark would be something like: msg = FooMessage()
# Read benchmark
msg.submsg = BarMessage()
1000000.times {
msg.submsg
}
# Write benchmark
1000000.times {
msg.submsg = BarMessage()
msg.submsg
} |
fowles
reviewed
Jun 16, 2023
This was referenced Jun 16, 2023
Superseeds: protocolbuffers#13054 The object cache is fundamentally subject to race conditions. Objects must be created before they are registered into the cache, so if two threads try to create the same object, we'll inevitably end up with two instances mapping to the same underlying memory. To entirely prevent that we'd need a lot of extra locking which I don't think is really worth it compared to a few useless allocations. Instead we can replace `ObjectCache_Add` by a `getset` type of operation, the extra instance is still created, but the later threads will receive the "canonical" instance and will be able to abandon their duplicated instance. Additionally, this PR moves the ObjectCache implementation in Ruby, as it's much easier to debug there, and the performance difference is negligible. The `ObjectCache` instance is also exposed as `Google::Protobuf::OBJECT_CACHE` to better allow to debug potential memory issues.
casperisfine
force-pushed
the
ruby-object-cache
branch
from
bcd2cfd to
cf2bbc3
Compare
Contributor
Author
So, note that there will necessarily be one. We're adding an extra method call on the fast path, and extra locking on the slow path. That said I'm extremely unfamiliar with protobuf, and I never used it (it's a transitive dependency of various Google Cloud gems), so I'd appreciate if one of you could take this over, as it would be faster than doing back and forth. I mostly opened this PR as to show a proof of concept. |
Contributor
|
Casper, thanks for providing it! I will take this over. |
mkruskal-google
added
🅰️ safe for tests
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Superseeds: #13054
The object cache is fundamentally subject to race conditions. Objects must be created before they are registered into the cache, so if two threads try to create the same object, we'll inevitably end up with two instances mapping to the same underlying memory.
To entirely prevent that we'd need a lot of extra locking which I don't think is really worth it compared to a few useless allocations.
Instead we can replace
ObjectCache_Addby agetsettype of operation, the extra instance is still created, but the later threads will receive the "canonical" instance and will be able to abandon their duplicated instance.Additionally, this PR moves the ObjectCache implementation in Ruby, as it's much easier to debug there, and the performance difference is negligible. The
ObjectCacheinstance is also exposed asGoogle::Protobuf::OBJECT_CACHEto better allow to debug potential memory issues.cc @haberman @fowles