From b75a49f9e02c65e8253e760cce39c41d344038e1 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Thu, 25 Mar 2021 10:45:15 -0700 Subject: [PATCH 1/4] GC secondary map periodically. --- ruby/ext/google/protobuf_c/protobuf.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/ruby/ext/google/protobuf_c/protobuf.c b/ruby/ext/google/protobuf_c/protobuf.c index c27f30aa2d55..06263192d256 100644 --- a/ruby/ext/google/protobuf_c/protobuf.c +++ b/ruby/ext/google/protobuf_c/protobuf.c @@ -251,14 +251,36 @@ void Arena_register(VALUE module) { // The object is used only for its identity; it does not contain any data. VALUE secondary_map = Qnil; +// Lambda that will GC entries from the secondary map that are no longer present +// in the primary map. +VALUE gc_secondary_map = Qnil; + +extern VALUE weak_obj_cache; + static void SecondaryMap_Init() { rb_gc_register_address(&secondary_map); + rb_gc_register_address(&gc_secondary_map); secondary_map = rb_hash_new(); + gc_secondary_map = rb_eval_string( + "->(secondary, weak) {\n" + " secondary.delete_if { |k, v| !weak.key?(v) }\n" + "}\n"); +} + +static void SecondaryMap_MaybeGC() { + size_t weak_len = NUM2ULL(rb_funcall(weak_obj_cache, rb_intern("length"), 0)); + size_t secondary_len = RHASH_SIZE(secondary_map); + size_t waste = secondary_len - weak_len; + PBRUBY_ASSERT(secondary_len >= weak_len); + if (waste > 1000 && waste > secondary_len * 0.2) { + rb_funcall(gc_secondary_map, rb_intern("call"), 2, secondary_map, weak_obj_cache); + } } static VALUE SecondaryMap_Get(VALUE key) { VALUE ret = rb_hash_lookup(secondary_map, key); if (ret == Qnil) { + SecondaryMap_MaybeGC(); ret = rb_eval_string("Object.new"); rb_hash_aset(secondary_map, key, ret); } From f0d6fcb2dadfcfd2ec073a5812baa2e3dae35c7f Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Thu, 25 Mar 2021 11:06:35 -0700 Subject: [PATCH 2/4] Wrap secondary map mutations in a mutex, to avoid mutation races. --- ruby/ext/google/protobuf_c/protobuf.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/ruby/ext/google/protobuf_c/protobuf.c b/ruby/ext/google/protobuf_c/protobuf.c index 06263192d256..6b9ae56569b7 100644 --- a/ruby/ext/google/protobuf_c/protobuf.c +++ b/ruby/ext/google/protobuf_c/protobuf.c @@ -251,6 +251,12 @@ void Arena_register(VALUE module) { // The object is used only for its identity; it does not contain any data. VALUE secondary_map = Qnil; +// Mutations to the map are under a mutex, because SeconaryMap_MaybeGC() +// iterates over the map which cannot happen in parallel with insertions, or +// Ruby will throw: +// can't add a new key into hash during iteration (RuntimeError) +VALUE secondary_map_mutex = Qnil; + // Lambda that will GC entries from the secondary map that are no longer present // in the primary map. VALUE gc_secondary_map = Qnil; @@ -260,11 +266,13 @@ extern VALUE weak_obj_cache; static void SecondaryMap_Init() { rb_gc_register_address(&secondary_map); rb_gc_register_address(&gc_secondary_map); + rb_gc_register_address(&secondary_map_mutex); secondary_map = rb_hash_new(); gc_secondary_map = rb_eval_string( "->(secondary, weak) {\n" " secondary.delete_if { |k, v| !weak.key?(v) }\n" "}\n"); + secondary_map_mutex = rb_mutex_new(); } static void SecondaryMap_MaybeGC() { @@ -280,9 +288,11 @@ static void SecondaryMap_MaybeGC() { static VALUE SecondaryMap_Get(VALUE key) { VALUE ret = rb_hash_lookup(secondary_map, key); if (ret == Qnil) { + rb_mutex_lock(secondary_map_mutex); SecondaryMap_MaybeGC(); ret = rb_eval_string("Object.new"); rb_hash_aset(secondary_map, key, ret); + rb_mutex_unlock(secondary_map_mutex); } return ret; } From e1ac3937258ebd9d33738fbed1ee631aa34e6f95 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Thu, 25 Mar 2021 11:34:29 -0700 Subject: [PATCH 3/4] Added some more comments and refactored slightly. --- ruby/ext/google/protobuf_c/protobuf.c | 21 +++++++++++++++++++-- ruby/ext/google/protobuf_c/protobuf.h | 2 ++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/ruby/ext/google/protobuf_c/protobuf.c b/ruby/ext/google/protobuf_c/protobuf.c index 6b9ae56569b7..850f5e9c11b1 100644 --- a/ruby/ext/google/protobuf_c/protobuf.c +++ b/ruby/ext/google/protobuf_c/protobuf.c @@ -260,6 +260,7 @@ VALUE secondary_map_mutex = Qnil; // Lambda that will GC entries from the secondary map that are no longer present // in the primary map. VALUE gc_secondary_map = Qnil; +ID length; extern VALUE weak_obj_cache; @@ -273,14 +274,30 @@ static void SecondaryMap_Init() { " secondary.delete_if { |k, v| !weak.key?(v) }\n" "}\n"); secondary_map_mutex = rb_mutex_new(); + length = rb_intern("length"); } +// The secondary map is a regular Hash, and will never shrink on its own. +// The main object cache is a WeakMap that will automatically remove entries +// when the target object is no longer reachable, but unless we manually +// remove the corresponding entries from the secondary map, it will grow +// without bound. +// +// To avoid this unbounded growth we periodically remove entries from the +// secondary map that are no longer present in the WeakMap. The logic of +// how often to perform this GC is an artbirary tuning parameter that +// represents a straightforward CPU/memory tradeoff. static void SecondaryMap_MaybeGC() { - size_t weak_len = NUM2ULL(rb_funcall(weak_obj_cache, rb_intern("length"), 0)); + size_t weak_len = NUM2ULL(rb_funcall(weak_obj_cache, length, 0)); size_t secondary_len = RHASH_SIZE(secondary_map); size_t waste = secondary_len - weak_len; PBRUBY_ASSERT(secondary_len >= weak_len); - if (waste > 1000 && waste > secondary_len * 0.2) { + // 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. + size_t threshold = PBRUBY_MAX(secondary_len * 0.2, 2000); + if (waste > threshold) { rb_funcall(gc_secondary_map, rb_intern("call"), 2, secondary_map, weak_obj_cache); } } diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index e4873b34d2bd..01f18fb3a43b 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/ruby/ext/google/protobuf_c/protobuf.h @@ -106,6 +106,8 @@ extern VALUE cTypeError; #define PBRUBY_ASSERT(expr) assert(expr) #endif +#define PBRUBY_MAX(x, y) (((x) > (y)) ? (x) : (y)) + #define UPB_UNUSED(var) (void)var #endif // __GOOGLE_PROTOBUF_RUBY_PROTOBUF_H__ From 2fe27d87641ec599ab2bfb0b6ec997e903655f48 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Mon, 29 Mar 2021 12:30:49 -0700 Subject: [PATCH 4/4] Addressed PR comments and fixed a bug. We now hold the mutex for both map insertions, to protect against a concurrent GC that removes from the seconary map before we can insert into the weak map. --- ruby/ext/google/protobuf_c/protobuf.c | 50 ++++++++++++++++++++------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/ruby/ext/google/protobuf_c/protobuf.c b/ruby/ext/google/protobuf_c/protobuf.c index 850f5e9c11b1..65263a44cf1b 100644 --- a/ruby/ext/google/protobuf_c/protobuf.c +++ b/ruby/ext/google/protobuf_c/protobuf.c @@ -259,17 +259,17 @@ VALUE secondary_map_mutex = Qnil; // Lambda that will GC entries from the secondary map that are no longer present // in the primary map. -VALUE gc_secondary_map = Qnil; +VALUE gc_secondary_map_lambda = Qnil; ID length; extern VALUE weak_obj_cache; static void SecondaryMap_Init() { rb_gc_register_address(&secondary_map); - rb_gc_register_address(&gc_secondary_map); + rb_gc_register_address(&gc_secondary_map_lambda); rb_gc_register_address(&secondary_map_mutex); secondary_map = rb_hash_new(); - gc_secondary_map = rb_eval_string( + gc_secondary_map_lambda = rb_eval_string( "->(secondary, weak) {\n" " secondary.delete_if { |k, v| !weak.key?(v) }\n" "}\n"); @@ -287,43 +287,61 @@ static void SecondaryMap_Init() { // secondary map that are no longer present in the WeakMap. The logic of // how often to perform this GC is an artbirary tuning parameter that // represents a straightforward CPU/memory tradeoff. +// +// Requires: secondary_map_mutex is held. static void SecondaryMap_MaybeGC() { + PBRUBY_ASSERT(rb_mutex_locked_p(secondary_map_mutex) == Qtrue); size_t weak_len = NUM2ULL(rb_funcall(weak_obj_cache, length, 0)); size_t secondary_len = RHASH_SIZE(secondary_map); + if (secondary_len < weak_len) { + // Logically this case should not be possible: a valid entry cannot exist in + // the weak table unless there is a corresponding entry in the secondary + // table. It should *always* be the case that secondary_len >= weak_len. + // + // However ObjectSpace::WeakMap#length (and therefore weak_len) is + // unreliable: it overreports its true length by including non-live objects. + // However these non-live objects are not yielded in iteration, so we may + // have previously deleted them from the secondary map in a previous + // invocation of SecondaryMap_MaybeGC(). + // + // In this case, we can't measure any waste, so we just return. + return; + } size_t waste = secondary_len - weak_len; - PBRUBY_ASSERT(secondary_len >= weak_len); // 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. size_t threshold = PBRUBY_MAX(secondary_len * 0.2, 2000); if (waste > threshold) { - rb_funcall(gc_secondary_map, rb_intern("call"), 2, secondary_map, weak_obj_cache); + rb_funcall(gc_secondary_map_lambda, rb_intern("call"), 2, + secondary_map, weak_obj_cache); } } -static VALUE SecondaryMap_Get(VALUE key) { +// Requires: secondary_map_mutex is held by this thread iff create == true. +static VALUE SecondaryMap_Get(VALUE key, bool create) { + PBRUBY_ASSERT(!create || rb_mutex_locked_p(secondary_map_mutex) == Qtrue); VALUE ret = rb_hash_lookup(secondary_map, key); - if (ret == Qnil) { - rb_mutex_lock(secondary_map_mutex); + if (ret == Qnil && create) { SecondaryMap_MaybeGC(); ret = rb_eval_string("Object.new"); rb_hash_aset(secondary_map, key, ret); - rb_mutex_unlock(secondary_map_mutex); } return ret; } #endif -static VALUE ObjectCache_GetKey(const void* key) { +// Requires: secondary_map_mutex is held by this thread iff create == true. +static VALUE ObjectCache_GetKey(const void* key, bool create) { char buf[sizeof(key)]; memcpy(&buf, &key, sizeof(key)); intptr_t key_int = (intptr_t)key; PBRUBY_ASSERT((key_int & 3) == 0); VALUE ret = LL2NUM(key_int >> 2); #if USE_SECONDARY_MAP - ret = SecondaryMap_Get(ret); + ret = SecondaryMap_Get(ret, create); #endif return ret; } @@ -347,14 +365,20 @@ static void ObjectCache_Init() { void ObjectCache_Add(const void* key, VALUE val) { PBRUBY_ASSERT(ObjectCache_Get(key) == Qnil); - VALUE key_rb = ObjectCache_GetKey(key); +#if USE_SECONDARY_MAP + rb_mutex_lock(secondary_map_mutex); +#endif + VALUE key_rb = ObjectCache_GetKey(key, true); rb_funcall(weak_obj_cache, item_set, 2, key_rb, val); +#if USE_SECONDARY_MAP + rb_mutex_unlock(secondary_map_mutex); +#endif PBRUBY_ASSERT(ObjectCache_Get(key) == val); } // Returns the cached object for this key, if any. Otherwise returns Qnil. VALUE ObjectCache_Get(const void* key) { - VALUE key_rb = ObjectCache_GetKey(key); + VALUE key_rb = ObjectCache_GetKey(key, false); return rb_funcall(weak_obj_cache, item_get, 1, key_rb); }