Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ruby: Refactor the object cache to better account for race conditions #13075

Closed

Conversation

casperisfine
Copy link
Contributor

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_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.

cc @haberman @fowles

@casperisfine casperisfine requested a review from a team as a code owner June 15, 2023 09:58
@casperisfine casperisfine requested review from haberman and removed request for a team June 15, 2023 09:58
ruby/ext/google/protobuf_c/protobuf.h Outdated Show resolved Hide resolved
ruby/ext/google/protobuf_c/protobuf.h Outdated Show resolved Hide resolved
ruby/ext/google/protobuf_c/repeated_field.c Outdated Show resolved Hide resolved
ruby/lib/google/protobuf.rb Show resolved Hide resolved
@casperisfine casperisfine force-pushed the ruby-object-cache branch 2 times, most recently from 74dd06f to bcd2cfd Compare June 16, 2023 15:32
@fowles fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jun 16, 2023
@haberman
Copy link
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
}

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
Copy link
Contributor Author

How do we validate that there is no performance regression?

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.

@fowles
Copy link
Member

fowles commented Jun 20, 2023

Casper, thanks for providing it! I will take this over.

@mkruskal-google mkruskal-google added 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Jun 26, 2023
copybara-service bot pushed a commit that referenced this pull request Jul 8, 2023
Supersedes  #13075

Closes #13204

COPYBARA_INTEGRATE_REVIEW=#13204 from fowles:main 9b6aad6
PiperOrigin-RevId: 546579459
@fowles fowles closed this Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🅰️ safe for tests Mark a commit as safe to run presubmits over
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants