ruby-changes:71837
From: Samuel <ko1@a...>
Date: Mon, 16 May 2022 21:50:28 +0900 (JST)
Subject: [ruby-changes:71837] f626998c4f (master): Delete autoload data from global features after autoload has completed. (#5910)
https://git.ruby-lang.org/ruby.git/commit/?id=f626998c4f From f626998c4fa62973cac3a027597f97cdacd0d3c5 Mon Sep 17 00:00:00 2001 From: Samuel Williams <samuel.williams@o...> Date: Tue, 17 May 2022 00:50:02 +1200 Subject: Delete autoload data from global features after autoload has completed. (#5910) * Update naming of critical section assertions macros. * Improved locking for autoload. --- test/ruby/test_autoload.rb | 28 +++ thread.c | 2 +- variable.c | 555 +++++++++++++++++++++++++-------------------- vm.c | 4 +- vm_core.h | 26 ++- 5 files changed, 353 insertions(+), 262 deletions(-) diff --git a/test/ruby/test_autoload.rb b/test/ruby/test_autoload.rb index 7709760d19..afabd5d26b 100644 --- a/test/ruby/test_autoload.rb +++ b/test/ruby/test_autoload.rb @@ -491,4 +491,32 @@ p Foo::Bar https://github.com/ruby/ruby/blob/trunk/test/ruby/test_autoload.rb#L491 ::Object.class_eval {remove_const(:AutoloadTest)} if defined? Object::AutoloadTest TestAutoload.class_eval {remove_const(:AutoloadTest)} if defined? TestAutoload::AutoloadTest end + + def test_autoload_parallel_race + Dir.mktmpdir('autoload') do |tmpdir| + autoload_path = File.join(tmpdir, "autoload_parallel_race.rb") + File.write(autoload_path, 'module Foo; end; module Bar; end') + + assert_separately([], <<-RUBY, timeout: 100) + autoload_path = #{File.realpath(autoload_path).inspect} + + # This should work with no errors or failures. + 1000.times do + autoload :Foo, autoload_path + autoload :Bar, autoload_path + + t1 = Thread.new {Foo} + t2 = Thread.new {Bar} + + t1.join + t2.join + + Object.send(:remove_const, :Foo) + Object.send(:remove_const, :Bar) + + $LOADED_FEATURES.delete(autoload_path) + end + RUBY + end + end end diff --git a/thread.c b/thread.c index 7582849767..bb9070f705 100644 --- a/thread.c +++ b/thread.c @@ -1563,7 +1563,7 @@ blocking_region_begin(rb_thread_t *th, struct rb_blocking_region_buffer *region, https://github.com/ruby/ruby/blob/trunk/thread.c#L1563 rb_unblock_function_t *ubf, void *arg, int fail_if_interrupted) { #ifdef RUBY_VM_CRITICAL_SECTION - VM_ASSERT(rb_vm_critical_section_entered == 0); + VM_ASSERT(ruby_assert_critical_section_entered == 0); #endif region->prev_status = th->status; diff --git a/variable.c b/variable.c index 15dc6b9fc0..ae482dfabe 100644 --- a/variable.c +++ b/variable.c @@ -49,8 +49,8 @@ static ID autoload, classpath, tmp_classpath; https://github.com/ruby/ruby/blob/trunk/variable.c#L49 // This hash table maps file paths to loadable features. We use this to track // autoload state until it's no longer needed. -// feature (file path) => struct autoload_i -static VALUE autoload_featuremap; +// feature (file path) => struct autoload_data +static VALUE autoload_features; // This mutex is used to protect autoloading state. We use a global mutex which // is held until a per-feature mutex can be created. This ensures there are no @@ -86,9 +86,9 @@ Init_var_tables(void) https://github.com/ruby/ruby/blob/trunk/variable.c#L86 rb_obj_hide(autoload_mutex); rb_gc_register_mark_object(autoload_mutex); - autoload_featuremap = rb_ident_hash_new(); - rb_obj_hide(autoload_featuremap); - rb_gc_register_mark_object(autoload_featuremap); + autoload_features = rb_ident_hash_new(); + rb_obj_hide(autoload_features); + rb_gc_register_mark_object(autoload_features); } static inline bool @@ -2073,38 +2073,38 @@ rb_mod_const_missing(VALUE klass, VALUE name) https://github.com/ruby/ruby/blob/trunk/variable.c#L2073 } static void -autoload_mark(void *ptr) +autoload_table_mark(void *ptr) { rb_mark_tbl_no_pin((st_table *)ptr); } static void -autoload_free(void *ptr) +autoload_table_free(void *ptr) { st_free_table((st_table *)ptr); } static size_t -autoload_memsize(const void *ptr) +autoload_table_memsize(const void *ptr) { const st_table *tbl = ptr; return st_memsize(tbl); } static void -autoload_compact(void *ptr) +autoload_table_compact(void *ptr) { rb_gc_update_tbl_refs((st_table *)ptr); } -static const rb_data_type_t autoload_data_type = { - "autoload", - {autoload_mark, autoload_free, autoload_memsize, autoload_compact,}, +static const rb_data_type_t autoload_table_type = { + "autoload_table", + {autoload_table_mark, autoload_table_free, autoload_table_memsize, autoload_table_compact,}, 0, 0, RUBY_TYPED_FREE_IMMEDIATELY }; #define check_autoload_table(av) \ - (struct st_table *)rb_check_typeddata((av), &autoload_data_type) + (struct st_table *)rb_check_typeddata((av), &autoload_table_type) static VALUE autoload_data(VALUE mod, ID id) @@ -2112,63 +2112,81 @@ autoload_data(VALUE mod, ID id) https://github.com/ruby/ruby/blob/trunk/variable.c#L2112 struct st_table *tbl; st_data_t val; - if (!st_lookup(RCLASS_IV_TBL(mod), autoload, &val) || - !(tbl = check_autoload_table((VALUE)val)) || - !st_lookup(tbl, (st_data_t)id, &val)) { - return 0; + // Look up the instance variable table for `autoload`, then index into that table with the given constant name `id`. + if (!st_lookup(RCLASS_IV_TBL(mod), autoload, &val) || !(tbl = check_autoload_table((VALUE)val)) || !st_lookup(tbl, (st_data_t)id, &val)) { + return 0; } + return (VALUE)val; } +// Every autoload constant has exactly one instance of autoload_const, stored in `autoload_features`. Since multiple autoload constants can refer to the same file, every `autoload_const` refers to a de-duplicated `autoload_data`. struct autoload_const { - struct ccan_list_node cnode; /* <=> autoload_data_i.constants */ - VALUE mod; - VALUE ad; /* autoload_data_i */ + // The linked list node of all constants which are loaded by the related autoload feature. + struct ccan_list_node cnode; /* <=> autoload_data.constants */ + + // The shared "autoload_data" if multiple constants are defined from the same feature. + VALUE autoload_data_value; + + // The module we are loading a constant into. + VALUE module; + + // The name of the constant we are loading. + VALUE name; + + // The value of the constant (after it's loaded). VALUE value; - VALUE file; - ID id; + + // The constant entry flags which need to be re-applied after autoloading the feature. rb_const_flag_t flag; + + // The source file and line number that defined this constant (different from feature path). + VALUE file; int line; }; -/* always on stack, no need to mark */ -struct autoload_state { - struct autoload_const *ac; - VALUE result; +// Each `autoload_data` uniquely represents a specific feature which can be loaded, and a list of constants which it is able to define. We use a mutex to coordinate multiple threads trying to load the same feature. +struct autoload_data { + // The feature path to require to load this constant. + VALUE feature; + + // The mutex which is protecting autoloading this feature. VALUE mutex; -}; -struct autoload_data_i { - VALUE feature; - struct autoload_state *state; /* points to on-stack struct */ + // The process fork serial number since the autoload mutex will become invalid on fork. rb_serial_t fork_gen; + + // The linked list of all constants that are going to be loaded by this autoload. struct ccan_list_head constants; /* <=> autoload_const.cnode */ }; static void -autoload_i_compact(void *ptr) +autoload_data_compact(void *ptr) { - struct autoload_data_i *p = ptr; + struct autoload_data *p = ptr; + p->feature = rb_gc_location(p->feature); + p->mutex = rb_gc_location(p->mutex); } static void -autoload_i_mark(void *ptr) +autoload_data_mark(void *ptr) { - struct autoload_data_i *p = ptr; + struct autoload_data *p = ptr; rb_gc_mark_movable(p->feature); + rb_gc_mark_movable(p->mutex); } static size_t -autoload_i_memsize(const void *ptr) +autoload_data_memsize(const void *ptr) { - return sizeof(struct autoload_data_i); + return sizeof(struct autoload_data); } -static const rb_data_type_t autoload_data_i_type = { - "autoload_i", - {autoload_i_mark, ruby_xfree, autoload_i_memsize, autoload_i_compact}, +static const rb_data_type_t autoload_data_type = { + "autoload_data", + {autoload_data_mark, ruby_xfree, autoload_data_memsize, autoload_data_compact}, 0, 0, RUBY_TYPED_FREE_IMMEDIATELY }; @@ -2177,8 +2195,8 @@ autoload_c_compact(void *ptr) https://github.com/ruby/ruby/blob/trunk/variable.c#L2195 { struct autoload_const *ac = ptr; - ac->mod = rb_gc_location(ac->mod); - ac->ad = rb_gc_location(ac->ad); + ac->module = rb_gc_location(ac->module); + ac->autoload_data_value = rb_gc_location(ac->autoload_data_value); ac->value = rb_gc_location(ac->value); ac->file = rb_gc_location(ac->file); } @@ -2188,8 +2206,8 @@ autoload_c_mark(void *ptr) https://github.com/ruby/ruby/blob/trunk/variable.c#L2206 { struct autoload_const *ac = ptr; - rb_gc_mark_movable(ac->mod); - rb_gc_mark_movable(ac->ad); + rb_gc_mark_movable(ac->module); + rb_gc_mark_movable(ac->autoload_data_value); rb_gc_mark_movable(ac->value); rb_gc_mark_movable(ac->file); } @@ -2206,16 +2224,16 @@ static const rb_data_type_t autoload_const_type = { https://github.com/ruby/ruby/blob/trunk/variable.c#L2224 0, 0, RUBY_TYPED_FREE_IMMEDIATELY }; -static struct autoload_data_i * +static struct autoload_data * get_autoload_data(VALUE acv, struct autoload_const **acp) { struct autoload_const *ac = rb_check_typeddata(acv, &autoload_const_type); - struct autoload_data_i *ele; + struct autoload_data *ele; - ele = rb_check_typeddata(ac->ad, &autoload_data_i_type); + ele = rb_check_typeddata(ac->autoload_data_value, &autoload_data_type (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/