ruby-changes:71842
From: Samuel <ko1@a...>
Date: Tue, 17 May 2022 16:12:52 +0900 (JST)
Subject: [ruby-changes:71842] 60d45b2ee8 (master): Restore implicit relationship between `autoload_const` and `autoload_data` during GC. (#5911)
https://git.ruby-lang.org/ruby.git/commit/?id=60d45b2ee8 From 60d45b2ee86a80e248c3bff0c90c981ed2168ac3 Mon Sep 17 00:00:00 2001 From: Samuel Williams <samuel.williams@o...> Date: Tue, 17 May 2022 19:12:36 +1200 Subject: Restore implicit relationship between `autoload_const` and `autoload_data` during GC. (#5911) --- test/ruby/test_autoload.rb | 20 +++++ variable.c | 178 +++++++++++++++++++++++++++++---------------- 2 files changed, 136 insertions(+), 62 deletions(-) diff --git a/test/ruby/test_autoload.rb b/test/ruby/test_autoload.rb index afabd5d26b..f6183f5ee2 100644 --- a/test/ruby/test_autoload.rb +++ b/test/ruby/test_autoload.rb @@ -492,6 +492,26 @@ p Foo::Bar https://github.com/ruby/ruby/blob/trunk/test/ruby/test_autoload.rb#L492 TestAutoload.class_eval {remove_const(:AutoloadTest)} if defined? TestAutoload::AutoloadTest end + def test_autoload_module_gc + Dir.mktmpdir('autoload') do |tmpdir| + autoload_path = File.join(tmpdir, "autoload_module_gc.rb") + File.write(autoload_path, "X = 1; Y = 2;") + + x = Module.new + x.autoload :X, "./feature.rb" + + 1000.times do + y = Module.new + y.autoload :Y, "./feature.rb" + end + + x = y = nil + + # Ensure the internal data structures are cleaned up correctly / don't crash: + GC.start + end + end + def test_autoload_parallel_race Dir.mktmpdir('autoload') do |tmpdir| autoload_path = File.join(tmpdir, "autoload_parallel_race.rb") diff --git a/variable.c b/variable.c index c56217f360..37359fe55a 100644 --- a/variable.c +++ b/variable.c @@ -2176,6 +2176,22 @@ autoload_data_mark(void *ptr) https://github.com/ruby/ruby/blob/trunk/variable.c#L2176 rb_gc_mark_movable(p->feature); rb_gc_mark_movable(p->mutex); + + // Allow GC to free us if no modules refer to this via autoload_const.autoload_data + if (ccan_list_empty(&p->constants)) { + rb_hash_delete(autoload_features, p->feature); + } +} + +static void +autoload_data_free(void *ptr) +{ + struct autoload_data *p = ptr; + + // We may leak some memory at VM shutdown time, no big deal...? + if (ccan_list_empty(&p->constants)) { + ruby_xfree(p); + } } static size_t @@ -2186,12 +2202,12 @@ autoload_data_memsize(const void *ptr) https://github.com/ruby/ruby/blob/trunk/variable.c#L2202 static const rb_data_type_t autoload_data_type = { "autoload_data", - {autoload_data_mark, ruby_xfree, autoload_data_memsize, autoload_data_compact}, + {autoload_data_mark, autoload_data_free, autoload_data_memsize, autoload_data_compact}, 0, 0, RUBY_TYPED_FREE_IMMEDIATELY }; static void -autoload_c_compact(void *ptr) +autoload_const_compact(void *ptr) { struct autoload_const *ac = ptr; @@ -2202,7 +2218,7 @@ autoload_c_compact(void *ptr) https://github.com/ruby/ruby/blob/trunk/variable.c#L2218 } static void -autoload_c_mark(void *ptr) +autoload_const_mark(void *ptr) { struct autoload_const *ac = ptr; @@ -2213,41 +2229,52 @@ autoload_c_mark(void *ptr) https://github.com/ruby/ruby/blob/trunk/variable.c#L2229 } static size_t -autoload_c_memsize(const void *ptr) +autoload_const_memsize(const void *ptr) { return sizeof(struct autoload_const); } +static void +autoload_const_free(void *ptr) +{ + struct autoload_const *autoload_const = ptr; + + ccan_list_del(&autoload_const->cnode); + ruby_xfree(ptr); +} + static const rb_data_type_t autoload_const_type = { "autoload_const", - {autoload_c_mark, ruby_xfree, autoload_c_memsize, autoload_c_compact,}, + {autoload_const_mark, autoload_const_free, autoload_const_memsize, autoload_const_compact,}, 0, 0, RUBY_TYPED_FREE_IMMEDIATELY }; static struct autoload_data * -get_autoload_data(VALUE acv, struct autoload_const **acp) +get_autoload_data(VALUE autoload_const_value, struct autoload_const **autoload_const_pointer) { - struct autoload_const *ac = rb_check_typeddata(acv, &autoload_const_type); - struct autoload_data *ele; + struct autoload_const *autoload_const = rb_check_typeddata(autoload_const_value, &autoload_const_type); + + struct autoload_data *autoload_data = rb_check_typeddata(autoload_const->autoload_data_value, &autoload_data_type); - ele = rb_check_typeddata(ac->autoload_data_value, &autoload_data_type); /* do not reach across stack for ->state after forking: */ - if (ele && ele->fork_gen != GET_VM()->fork_gen) { - ele->mutex = Qnil; - ele->fork_gen = 0; + if (autoload_data && autoload_data->fork_gen != GET_VM()->fork_gen) { + autoload_data->mutex = Qnil; + autoload_data->fork_gen = 0; } - if (acp) *acp = ac; - return ele; + + if (autoload_const_pointer) *autoload_const_pointer = autoload_const; + + return autoload_data; } RUBY_FUNC_EXPORTED void -rb_autoload(VALUE mod, ID id, const char *file) +rb_autoload(VALUE module, ID name, const char *feature) { - if (!file || !*file) { - rb_raise(rb_eArgError, "empty file name"); + if (!feature || !*feature) { + rb_raise(rb_eArgError, "empty feature name"); } - rb_autoload_str(mod, id, rb_fstring_cstr(file)); + rb_autoload_str(module, name, rb_fstring_cstr(feature)); } static void const_set(VALUE klass, ID id, VALUE val); @@ -2256,25 +2283,47 @@ static void const_added(VALUE klass, ID const_name); https://github.com/ruby/ruby/blob/trunk/variable.c#L2283 struct autoload_arguments { VALUE module; ID name; - - VALUE path; + VALUE feature; }; static VALUE -autoload_feature_lookup_or_create(VALUE file) +autoload_feature_lookup_or_create(VALUE feature, struct autoload_data **autoload_data_pointer) { - VALUE ad = rb_hash_aref(autoload_features, file); - struct autoload_data *ele; + RUBY_ASSERT_MUTEX_OWNED(autoload_mutex); + RUBY_ASSERT_CRITICAL_SECTION_ENTER(); - if (NIL_P(ad)) { - ad = TypedData_Make_Struct(0, struct autoload_data, &autoload_data_type, ele); - ele->feature = file; - ele->mutex = Qnil; - ccan_list_head_init(&ele->constants); - rb_hash_aset(autoload_features, file, ad); + VALUE autoload_data_value = rb_hash_aref(autoload_features, feature); + struct autoload_data *autoload_data; + + if (NIL_P(autoload_data_value)) { + autoload_data_value = TypedData_Make_Struct(0, struct autoload_data, &autoload_data_type, autoload_data); + autoload_data->feature = feature; + autoload_data->mutex = Qnil; + ccan_list_head_init(&autoload_data->constants); + + if (autoload_data_pointer) *autoload_data_pointer = autoload_data; + + rb_hash_aset(autoload_features, feature, autoload_data_value); + } else if (autoload_data_pointer) { + *autoload_data_pointer = rb_check_typeddata(autoload_data_value, &autoload_data_type); } - return ad; + RUBY_ASSERT_CRITICAL_SECTION_LEAVE(); + return autoload_data_value; +} + +static VALUE +autoload_feature_clear_if_empty(VALUE argument) +{ + RUBY_ASSERT_MUTEX_OWNED(autoload_mutex); + + struct autoload_data *autoload_data = (struct autoload_data *)argument; + + if (ccan_list_empty(&autoload_data->constants)) { + rb_hash_delete(autoload_features, autoload_data->feature); + } + + return Qnil; } static struct st_table* autoload_table_lookup_or_create(VALUE module) { @@ -2313,10 +2362,10 @@ autoload_synchronized(VALUE _arguments) https://github.com/ruby/ruby/blob/trunk/variable.c#L2362 struct st_table *autoload_table = autoload_table_lookup_or_create(arguments->module); // Ensure the string is uniqued since we use an identity lookup: - VALUE path = rb_fstring(arguments->path); + VALUE feature = rb_fstring(arguments->feature); - VALUE autoload_data_value = autoload_feature_lookup_or_create(path); - struct autoload_data *autoload_data = rb_check_typeddata(autoload_data_value, &autoload_data_type); + struct autoload_data *autoload_data; + VALUE autoload_data_value = autoload_feature_lookup_or_create(feature, &autoload_data); { struct autoload_const *autoload_const; @@ -2334,59 +2383,63 @@ autoload_synchronized(VALUE _arguments) https://github.com/ruby/ruby/blob/trunk/variable.c#L2383 } void -rb_autoload_str(VALUE mod, ID id, VALUE file) +rb_autoload_str(VALUE module, ID name, VALUE feature) { - if (!rb_is_const_id(id)) { - rb_raise(rb_eNameError, "autoload must be constant name: %"PRIsVALUE"", QUOTE_ID(id)); + if (!rb_is_const_id(name)) { + rb_raise(rb_eNameError, "autoload must be constant name: %"PRIsVALUE"", QUOTE_ID(name)); } - Check_Type(file, T_STRING); - if (!RSTRING_LEN(file)) { - rb_raise(rb_eArgError, "empty file name"); + Check_Type(feature, T_STRING); + if (!RSTRING_LEN(feature)) { + rb_raise(rb_eArgError, "empty feature name"); } struct autoload_arguments arguments = { - .module = mod, - .name = id, - .path = file, + .module = module, + .name = name, + .feature = feature, }; VALUE result = rb_mutex_synchronize(autoload_mutex, autoload_synchronized, (VALUE)&arguments); if (result == Qtrue) { - const_added(mod, id); + const_added(module, name); } } static void -autoload_delete(VALUE mod, ID id) +autoload_delete(VALUE module, ID name) { - st_data_t val, load = 0, n = id; + RUBY_ASSERT_CRITICAL_SECTION_ENTER(); + + st_data_t value, load = 0, key = name; - if (st_lookup(RCLASS_IV_TBL(mod), (st_data_t)autoload, &val)) { - struct st_table *tbl = check_autoload_table((VALUE)val); - struct autoload_data *ele; - struct autoload_const *ac; + if (st_lookup(RCLASS_IV_TBL(module), (st_data_t)autoload, &value)) { + struct st_table *table = check_autoload_table((VALUE)value); - st_delete(tbl, &n, &load); + st_delete(table, &key, &load); /* Qfalse can indicate already deleted */ i (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/