From 9087337e5103d058887c691913b08182ea8da899 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Tue, 13 Feb 2024 09:31:59 -0800 Subject: [PATCH] Moved ObjectCache into an internal module. This type has always been internal-only, but moving it to a module named `Internal` will make this clearer. PiperOrigin-RevId: 606649281 --- ruby/ext/google/protobuf_c/protobuf.c | 11 ++- ruby/lib/google/protobuf.rb | 2 +- ruby/lib/google/protobuf/ffi/object_cache.rb | 6 +- .../google/protobuf/internal/object_cache.rb | 99 +++++++++++++++++++ ruby/lib/google/protobuf/object_cache.rb | 97 ------------------ ruby/tests/object_cache_test.rb | 17 ++-- 6 files changed, 117 insertions(+), 115 deletions(-) create mode 100644 ruby/lib/google/protobuf/internal/object_cache.rb delete mode 100644 ruby/lib/google/protobuf/object_cache.rb diff --git a/ruby/ext/google/protobuf_c/protobuf.c b/ruby/ext/google/protobuf_c/protobuf.c index c15a1caa6c..ad6239ee3f 100644 --- a/ruby/ext/google/protobuf_c/protobuf.c +++ b/ruby/ext/google/protobuf_c/protobuf.c @@ -241,16 +241,17 @@ static void ObjectCache_Init(VALUE protobuf) { item_try_add = rb_intern("try_add"); rb_gc_register_address(&weak_obj_cache); + VALUE internal = rb_const_get(protobuf, rb_intern("Internal")); #if SIZEOF_LONG >= SIZEOF_VALUE - VALUE cache_class = rb_const_get(protobuf, rb_intern("ObjectCache")); + VALUE cache_class = rb_const_get(internal, rb_intern("ObjectCache")); #else - VALUE cache_class = rb_const_get(protobuf, rb_intern("LegacyObjectCache")); + VALUE cache_class = rb_const_get(internal, rb_intern("LegacyObjectCache")); #endif weak_obj_cache = rb_class_new_instance(0, NULL, cache_class); - rb_const_set(protobuf, rb_intern("OBJECT_CACHE"), weak_obj_cache); - rb_const_set(protobuf, rb_intern("SIZEOF_LONG"), INT2NUM(SIZEOF_LONG)); - rb_const_set(protobuf, rb_intern("SIZEOF_VALUE"), INT2NUM(SIZEOF_VALUE)); + rb_const_set(internal, rb_intern("OBJECT_CACHE"), weak_obj_cache); + rb_const_set(internal, rb_intern("SIZEOF_LONG"), INT2NUM(SIZEOF_LONG)); + rb_const_set(internal, rb_intern("SIZEOF_VALUE"), INT2NUM(SIZEOF_VALUE)); } static VALUE ObjectCache_GetKey(const void *key) { diff --git a/ruby/lib/google/protobuf.rb b/ruby/lib/google/protobuf.rb index 20570a08df..fdfa9b687e 100644 --- a/ruby/lib/google/protobuf.rb +++ b/ruby/lib/google/protobuf.rb @@ -7,7 +7,7 @@ # require mixins before we hook them into the java & c code require 'google/protobuf/message_exts' -require 'google/protobuf/object_cache' +require 'google/protobuf/internal/object_cache' # We define these before requiring the platform-specific modules. # That way the module init can grab references to these. diff --git a/ruby/lib/google/protobuf/ffi/object_cache.rb b/ruby/lib/google/protobuf/ffi/object_cache.rb index ff287115a0..61f5c8e7f4 100644 --- a/ruby/lib/google/protobuf/ffi/object_cache.rb +++ b/ruby/lib/google/protobuf/ffi/object_cache.rb @@ -18,13 +18,13 @@ module Google def self.cache_implementation if interpreter_supports_non_finalized_keys_in_weak_map? and SIZEOF_LONG >= SIZEOF_VALUE - Google::Protobuf::ObjectCache + Google::Protobuf::Internal::ObjectCache else - Google::Protobuf::LegacyObjectCache + Google::Protobuf::Internal::LegacyObjectCache end end public OBJECT_CACHE = cache_implementation.new end -end \ No newline at end of file +end diff --git a/ruby/lib/google/protobuf/internal/object_cache.rb b/ruby/lib/google/protobuf/internal/object_cache.rb new file mode 100644 index 0000000000..38e9bb5fe9 --- /dev/null +++ b/ruby/lib/google/protobuf/internal/object_cache.rb @@ -0,0 +1,99 @@ +# Protocol Buffers - Google's data interchange format +# Copyright 2023 Google Inc. All rights reserved. +# +# Use of this source code is governed by a BSD-style +# license that can be found in the LICENSE file or at +# https://developers.google.com/open-source/licenses/bsd + +module Google + module Protobuf + module Internal + # A pointer -> Ruby Object cache that keeps references to Ruby wrapper + # objects. This allows us to look up any Ruby wrapper object by the address + # of the object it is wrapping. That way we can avoid ever creating two + # different wrapper objects for the same C object, which saves memory and + # preserves object identity. + # + # We use WeakMap for the cache. If sizeof(long) > sizeof(VALUE), we also + # need a secondary Hash to store WeakMap keys, because our pointer keys may + # need to be stored as Bignum instead of Fixnum. Since WeakMap is weak for + # both keys and values, a Bignum key will cause the WeakMap entry to be + # collected immediately unless there is another reference to the Bignum. + # This happens on 64-bit Windows, on which pointers are 64 bits but longs + # are 32 bits. In this case, we enable the secondary Hash to hold the keys + # and prevent them from being collected. + class ObjectCache + def initialize + @map = ObjectSpace::WeakMap.new + @mutex = Mutex.new + end + + def get(key) + @map[key] + end + + def try_add(key, value) + @map[key] || @mutex.synchronize do + @map[key] ||= value + end + end + end + + class LegacyObjectCache + def initialize + @secondary_map = {} + @map = ObjectSpace::WeakMap.new + @mutex = Mutex.new + end + + def get(key) + value = if secondary_key = @secondary_map[key] + @map[secondary_key] + else + @mutex.synchronize do + @map[(@secondary_map[key] ||= Object.new)] + end + end + + # GC if we could remove at least 2000 entries or 20% of the table size + # (whichever is greater). Since the cost of the GC pass is O(N), we + # want to make sure that we condition this on overall table size, to + # avoid O(N^2) CPU costs. + cutoff = (@secondary_map.size * 0.2).ceil + cutoff = 2_000 if cutoff < 2_000 + if (@secondary_map.size - @map.size) > cutoff + purge + end + + value + end + + def try_add(key, value) + if secondary_key = @secondary_map[key] + if old_value = @map[secondary_key] + return old_value + end + end + + @mutex.synchronize do + secondary_key ||= (@secondary_map[key] ||= Object.new) + @map[secondary_key] ||= value + end + end + + private + + def purge + @mutex.synchronize do + @secondary_map.each do |key, secondary_key| + unless @map.key?(secondary_key) + @secondary_map.delete(key) + end + end + end + nil + end + end + end + end +end diff --git a/ruby/lib/google/protobuf/object_cache.rb b/ruby/lib/google/protobuf/object_cache.rb deleted file mode 100644 index d99b2a74be..0000000000 --- a/ruby/lib/google/protobuf/object_cache.rb +++ /dev/null @@ -1,97 +0,0 @@ -# Protocol Buffers - Google's data interchange format -# Copyright 2023 Google Inc. All rights reserved. -# -# Use of this source code is governed by a BSD-style -# license that can be found in the LICENSE file or at -# https://developers.google.com/open-source/licenses/bsd - -module Google - module Protobuf - # A pointer -> Ruby Object cache that keeps references to Ruby wrapper - # objects. This allows us to look up any Ruby wrapper object by the address - # of the object it is wrapping. That way we can avoid ever creating two - # different wrapper objects for the same C object, which saves memory and - # preserves object identity. - # - # We use WeakMap for the cache. If sizeof(long) > sizeof(VALUE), we also - # need a secondary Hash to store WeakMap keys, because our pointer keys may - # need to be stored as Bignum instead of Fixnum. Since WeakMap is weak for - # both keys and values, a Bignum key will cause the WeakMap entry to be - # collected immediately unless there is another reference to the Bignum. - # This happens on 64-bit Windows, on which pointers are 64 bits but longs - # are 32 bits. In this case, we enable the secondary Hash to hold the keys - # and prevent them from being collected. - class ObjectCache - def initialize - @map = ObjectSpace::WeakMap.new - @mutex = Mutex.new - end - - def get(key) - @map[key] - end - - def try_add(key, value) - @map[key] || @mutex.synchronize do - @map[key] ||= value - end - end - end - - class LegacyObjectCache - def initialize - @secondary_map = {} - @map = ObjectSpace::WeakMap.new - @mutex = Mutex.new - end - - def get(key) - value = if secondary_key = @secondary_map[key] - @map[secondary_key] - else - @mutex.synchronize do - @map[(@secondary_map[key] ||= Object.new)] - end - end - - # GC if we could remove at least 2000 entries or 20% of the table size - # (whichever is greater). Since the cost of the GC pass is O(N), we - # want to make sure that we condition this on overall table size, to - # avoid O(N^2) CPU costs. - cutoff = (@secondary_map.size * 0.2).ceil - cutoff = 2_000 if cutoff < 2_000 - if (@secondary_map.size - @map.size) > cutoff - purge - end - - value - end - - def try_add(key, value) - if secondary_key = @secondary_map[key] - if old_value = @map[secondary_key] - return old_value - end - end - - @mutex.synchronize do - secondary_key ||= (@secondary_map[key] ||= Object.new) - @map[secondary_key] ||= value - end - end - - private - - def purge - @mutex.synchronize do - @secondary_map.each do |key, secondary_key| - unless @map.key?(secondary_key) - @secondary_map.delete(key) - end - end - end - nil - end - end - end -end diff --git a/ruby/tests/object_cache_test.rb b/ruby/tests/object_cache_test.rb index 1d9b77f9e7..1125e9e88b 100644 --- a/ruby/tests/object_cache_test.rb +++ b/ruby/tests/object_cache_test.rb @@ -3,11 +3,11 @@ require 'test/unit' class PlatformTest < Test::Unit::TestCase def test_correct_implementation_for_platform - omit('OBJECT_CACHE not defined') unless defined? Google::Protobuf::OBJECT_CACHE - if Google::Protobuf::SIZEOF_LONG >= Google::Protobuf::SIZEOF_VALUE and not defined? JRUBY_VERSION - assert_instance_of Google::Protobuf::ObjectCache, Google::Protobuf::OBJECT_CACHE + omit('OBJECT_CACHE not defined') unless defined? Google::Protobuf::Internal::OBJECT_CACHE + if Google::Protobuf::Internal::SIZEOF_LONG >= Google::Protobuf::Internal::SIZEOF_VALUE and not defined? JRUBY_VERSION + assert_instance_of Google::Protobuf::Internal::ObjectCache, Google::Protobuf::Internal::OBJECT_CACHE else - assert_instance_of Google::Protobuf::LegacyObjectCache, Google::Protobuf::OBJECT_CACHE + assert_instance_of Google::Protobuf::Internal::LegacyObjectCache, Google::Protobuf::Internal::OBJECT_CACHE end end end @@ -47,8 +47,8 @@ end class ObjectCacheTest < Test::Unit::TestCase def create - omit('OBJECT_CACHE not defined') unless defined? Google::Protobuf::OBJECT_CACHE - Google::Protobuf::ObjectCache.new + omit('OBJECT_CACHE not defined') unless defined? Google::Protobuf::Internal::OBJECT_CACHE + Google::Protobuf::Internal::ObjectCache.new end include ObjectCacheTestModule @@ -56,10 +56,9 @@ end class LegacyObjectCacheTest < Test::Unit::TestCase def create - omit('OBJECT_CACHE not defined') unless defined? Google::Protobuf::OBJECT_CACHE - Google::Protobuf::LegacyObjectCache.new + omit('OBJECT_CACHE not defined') unless defined? Google::Protobuf::Internal::OBJECT_CACHE + Google::Protobuf::Internal::LegacyObjectCache.new end include ObjectCacheTestModule end -