Refactor the object cache to better account for race conditions #13204

fowles · 2023-07-04T19:23:04Z

Supersedes #13075

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.

ruby/lib/google/protobuf.rb

ruby/ext/google/protobuf_c/protobuf.h

ruby/lib/google/protobuf/object_cache.rb

ruby/ext/google/protobuf_c/protobuf.c

ruby/ext/google/protobuf_c/message.c

ruby/ext/google/protobuf_c/repeated_field.c

JasonLunn · 2023-07-05T18:01:19Z

Should unit tests be added to verify any of the following:

Selection of the cache implementation on the current platform
Standalone behavior of both ObjectCache implementations outside of the side effects / asserts elsewhere in the runtime
Verification of the synchronization / safety properties of both caches
Verification of the garbage collection / memory consumption properties of the legacy cache

Add unit tests for ObjectCache.

responded to comments already

protocolbuffers#13204 refactored the Ruby object cache to use a key of `LL2NUM(key_val)` instead of `LL2NUM(key_val >> 2)`. On 32-bit systems, it appears that `LL2NUM(key_val)` returns inconsistent results, possibly due to overflow. This causes cache lookups to fail. This commit restores the previous behavior of using `ObjectCache_GetKey`, which discards the lower 2 bits, which are zero. Closes protocolbuffers#13481

protocolbuffers#13204 refactored the Ruby object cache to use a key of `LL2NUM(key_val)` instead of `LL2NUM(key_val >> 2)`. On 32-bit systems, LL2NUM(key_val) returns inconsistent results because a large value has to be stored as a Bignum on the heap. This causes cache lookups to fail. This commit restores the previous behavior of using `ObjectCache_GetKey`, which discards the lower 2 bits, which are zero. This enables a key to be stored as a Fixnum on both 32 and 64-bit platforms. Closes protocolbuffers#13481

protocolbuffers#13204 refactored the Ruby object cache to use a key of `LL2NUM(key_val)` instead of `LL2NUM(key_val >> 2)`. On 32-bit systems, `LL2NUM(key_val)` returns inconsistent results because a large value has to be stored as a Bignum on the heap. This causes cache lookups to fail. This commit restores the previous behavior of using `ObjectCache_GetKey`, which discards the lower 2 bits, which are zero. This enables a key to be stored as a Fixnum on both 32 and 64-bit platforms. As https://patshaughnessy.net/2014/1/9/how-big-is-a-bignum describes, a Fixnum uses: * 1 bit for the `FIXNUM_FLAG`. * 1 bit for the sign bit. Therefore the largest possible Fixnum value on a 64-bit value is 4611686018427387903 (2^62 - 1). On a 32-bit system, the largest value is 1073741823 (2^30 - 1). For example, a possible VALUE pointer address on a 32-bit system: 0xff5b4af8 => 4284173048 Dropping the lower 2 bits makes up for the loss of range to these flags. In the example above, we see that shifting by 2 turns the value into a 30-bit number, which can be represented as a Fixnum: (0xff5b4af8 >> 2) => 1071043262 This bug can also manifest on a 64-bit system if the upper bits are 0xff. Closes protocolbuffers#13481

#13204 refactored the Ruby object cache to use a key of `LL2NUM(key_val)` instead of `LL2NUM(key_val >> 2)`. On 32-bit systems, `LL2NUM(key_val)` returns inconsistent results because a large value has to be stored as a Bignum on the heap. This causes cache lookups to fail. This commit restores the previous behavior of using `ObjectCache_GetKey`, which discards the lower 2 bits, which are zero. This enables a key to be stored as a Fixnum on both 32 and 64-bit platforms. As https://patshaughnessy.net/2014/1/9/how-big-is-a-bignum describes, a Fixnum uses: * 1 bit for the `FIXNUM_FLAG`. * 1 bit for the sign flag. Therefore the largest possible Fixnum value on a 64-bit value is 4611686018427387903 (2^62 - 1). On a 32-bit system, the largest value is 1073741823 (2^30 - 1). For example, a possible VALUE pointer address on a 32-bit system: 0xff5b4af8 => 4284173048 Dropping the lower 2 bits makes up for the loss of range to these flags. In the example above, we see that shifting by 2 turns the value into a 30-bit number, which can be represented as a Fixnum: (0xff5b4af8 >> 2) => 1071043262 This bug can also manifest on a 64-bit system if the upper bits are 0xff. Closes #13481 Closes #13494 COPYBARA_INTEGRATE_REVIEW=#13494 from stanhu:sh-fix-ruby-protobuf-32bit d63122a FUTURE_COPYBARA_INTEGRATE_REVIEW=#13494 from stanhu:sh-fix-ruby-protobuf-32bit d63122a PiperOrigin-RevId: 557189479

#13204 refactored the Ruby object cache to use a key of `LL2NUM(key_val)` instead of `LL2NUM(key_val >> 2)`. On 32-bit systems, `LL2NUM(key_val)` returns inconsistent results because a large value has to be stored as a Bignum on the heap. This causes cache lookups to fail. This commit restores the previous behavior of using `ObjectCache_GetKey`, which discards the lower 2 bits, which are zero. This enables a key to be stored as a Fixnum on both 32 and 64-bit platforms. As https://patshaughnessy.net/2014/1/9/how-big-is-a-bignum describes, a Fixnum uses: * 1 bit for the `FIXNUM_FLAG`. * 1 bit for the sign flag. Therefore the largest possible Fixnum value on a 64-bit value is 4611686018427387903 (2^62 - 1). On a 32-bit system, the largest value is 1073741823 (2^30 - 1). For example, a possible VALUE pointer address on a 32-bit system: 0xff5b4af8 => 4284173048 Dropping the lower 2 bits makes up for the loss of range to these flags. In the example above, we see that shifting by 2 turns the value into a 30-bit number, which can be represented as a Fixnum: (0xff5b4af8 >> 2) => 1071043262 This bug can also manifest on a 64-bit system if the upper bits are 0xff. Closes #13481 Closes #13494 COPYBARA_INTEGRATE_REVIEW=#13494 from stanhu:sh-fix-ruby-protobuf-32bit d63122a FUTURE_COPYBARA_INTEGRATE_REVIEW=#13494 from stanhu:sh-fix-ruby-protobuf-32bit d63122a PiperOrigin-RevId: 557216800

#13204 refactored the Ruby object cache to use a key of `LL2NUM(key_val)` instead of `LL2NUM(key_val >> 2)`. On 32-bit systems, `LL2NUM(key_val)` returns inconsistent results because a large value has to be stored as a Bignum on the heap. This causes cache lookups to fail. This commit restores the previous behavior of using `ObjectCache_GetKey`, which discards the lower 2 bits, which are zero. This enables a key to be stored as a Fixnum on both 32 and 64-bit platforms. As https://patshaughnessy.net/2014/1/9/how-big-is-a-bignum describes, a Fixnum uses: * 1 bit for the `FIXNUM_FLAG`. * 1 bit for the sign flag. Therefore the largest possible Fixnum value on a 64-bit value is 4611686018427387903 (2^62 - 1). On a 32-bit system, the largest value is 1073741823 (2^30 - 1). For example, a possible VALUE pointer address on a 32-bit system: 0xff5b4af8 => 4284173048 Dropping the lower 2 bits makes up for the loss of range to these flags. In the example above, we see that shifting by 2 turns the value into a 30-bit number, which can be represented as a Fixnum: (0xff5b4af8 >> 2) => 1071043262 This bug can also manifest on a 64-bit system if the upper bits are 0xff. Closes #13481 Closes #13494 COPYBARA_INTEGRATE_REVIEW=#13494 from stanhu:sh-fix-ruby-protobuf-32bit d63122a PiperOrigin-RevId: 557211768

#13204 refactored the Ruby object cache to use a key of `LL2NUM(key_val)` instead of `LL2NUM(key_val >> 2)`. On 32-bit systems, `LL2NUM(key_val)` returns inconsistent results because a large value has to be stored as a Bignum on the heap. This causes cache lookups to fail. This commit restores the previous behavior of using `ObjectCache_GetKey`, which discards the lower 2 bits, which are zero. This enables a key to be stored as a Fixnum on both 32 and 64-bit platforms. As https://patshaughnessy.net/2014/1/9/how-big-is-a-bignum describes, a Fixnum uses: * 1 bit for the `FIXNUM_FLAG`. * 1 bit for the sign flag. Therefore the largest possible Fixnum value on a 64-bit value is 4611686018427387903 (2^62 - 1). On a 32-bit system, the largest value is 1073741823 (2^30 - 1). For example, a possible VALUE pointer address on a 32-bit system: 0xff5b4af8 => 4284173048 Dropping the lower 2 bits makes up for the loss of range to these flags. In the example above, we see that shifting by 2 turns the value into a 30-bit number, which can be represented as a Fixnum: (0xff5b4af8 >> 2) => 1071043262 This bug can also manifest on a 64-bit system if the upper bits are 0xff. Closes #13481 Closes #13494 COPYBARA_INTEGRATE_REVIEW=#13494 from stanhu:sh-fix-ruby-protobuf-32bit d63122a PiperOrigin-RevId: 557211768 Co-authored-by: Stan Hu <stanhu@gmail.com>

byroot and others added 3 commits July 4, 2023 10:09

ignore twiddle files

a7fcb44

add extra asserts

ee7a450

fowles marked this pull request as ready for review July 4, 2023 19:23

fowles requested a review from a team as a code owner July 4, 2023 19:23

fowles requested review from haberman and removed request for a team July 4, 2023 19:23

fowles added ruby 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Jul 4, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 4, 2023

fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 4, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 4, 2023

JasonLunn reviewed Jul 5, 2023

View reviewed changes

ruby/lib/google/protobuf.rb Outdated Show resolved Hide resolved

split object_cache into its own file

c55775b

fowles requested review from mkruskal-google and removed request for haberman July 5, 2023 16:21

fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 5, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 5, 2023

mkruskal-google reviewed Jul 5, 2023

View reviewed changes

ruby/ext/google/protobuf_c/protobuf.h Outdated Show resolved Hide resolved

JasonLunn previously requested changes Jul 5, 2023

View reviewed changes

fowles added 2 commits July 5, 2023 14:16

mark purge private

3f82dfe

rename getset to tryadd

999ab1a

fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 5, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 5, 2023

mkruskal-google approved these changes Jul 5, 2023

View reviewed changes

JasonLunn and others added 2 commits July 7, 2023 18:15

Add unit tests for ObjectCache.

e40194c

Merge pull request #1 from JasonLunn/for_kfm

637ffdb

Add unit tests for ObjectCache.

fowles requested a review from a team as a code owner July 7, 2023 22:35

fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 7, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 8, 2023

fowles added platform related Any issue releated to specific platform or OS 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Jul 8, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 8, 2023

Improve coverage by testing Legacy on modern Ruby

19f6d4b

fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 8, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 8, 2023

disable all tests on jruby

9b6aad6

fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 8, 2023

github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jul 8, 2023

fowles requested a review from mkruskal-google July 8, 2023 04:33

mkruskal-google approved these changes Jul 8, 2023

View reviewed changes

copybara-service bot closed this in a01d047 Jul 8, 2023

haberman mentioned this pull request Jul 18, 2023

ruby: segfault iterating over protobuf objects #11968

Closed

This was referenced Aug 9, 2023

[ruby] Unable to instantiate a descriptor in v3.24.0 on 32-bit system #13481

Closed

ruby: Fix object cache lookups on 32-bit platforms #13494

Closed

copybara-service bot mentioned this pull request Aug 15, 2023

ruby: Fix object cache lookups on 32-bit platforms (#13494) #13551

Closed

copybara-service bot mentioned this pull request Aug 15, 2023

ruby: Fix object cache lookups on 32-bit platforms (#13494) #13557

Closed

esrauchg mentioned this pull request Aug 17, 2023

ruby: Fix object cache lookups on 32-bit platforms (#13494) #13580

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the object cache to better account for race conditions #13204

Refactor the object cache to better account for race conditions #13204

fowles commented Jul 4, 2023

JasonLunn commented Jul 5, 2023

Refactor the object cache to better account for race conditions #13204

Refactor the object cache to better account for race conditions #13204

Conversation

fowles commented Jul 4, 2023

JasonLunn commented Jul 5, 2023