ruby-changes:71512
From: Nobuyoshi <ko1@a...>
Date: Fri, 25 Mar 2022 20:29:20 +0900 (JST)
Subject: [ruby-changes:71512] 69967ee64e (master): Revert "Finer-grained inline constant cache invalidation"
https://git.ruby-lang.org/ruby.git/commit/?id=69967ee64e From 69967ee64eac9ce65b83533a566d69d12a6046d0 Mon Sep 17 00:00:00 2001 From: Nobuyoshi Nakada <nobu@r...> Date: Fri, 25 Mar 2022 20:29:09 +0900 Subject: Revert "Finer-grained inline constant cache invalidation" This reverts commits for [Feature #18589]: * 8008fb7352abc6fba433b99bf20763cf0d4adb38 "Update formatting per feedback" * 8f6eaca2e19828e92ecdb28b0fe693d606a03f96 "Delete ID from constant cache table if it becomes empty on ISEQ free" * 629908586b4bead1103267652f8b96b1083573a8 "Finer-grained inline constant cache invalidation" MSWin builds on AppVeyor have been crashing since the merger. --- benchmark/constant_invalidation.rb | 22 --- bootstraptest/test_constant_cache.rb | 187 --------------------- class.c | 16 +- include/ruby/backward.h | 2 + include/ruby/internal/intern/vm.h | 7 - insns.def | 13 +- internal/vm.h | 1 + iseq.c | 90 ---------- iseq.h | 3 - test/ruby/test_rubyvm.rb | 4 +- .../ruby_vm/views/_mjit_compile_getinlinecache.erb | 4 +- variable.c | 19 ++- vm.c | 61 +------ vm_core.h | 40 ++++- vm_insnhelper.c | 51 +----- vm_insnhelper.h | 3 + vm_method.c | 20 +-- yjit_codegen.c | 5 +- 18 files changed, 82 insertions(+), 466 deletions(-) delete mode 100644 benchmark/constant_invalidation.rb delete mode 100644 bootstraptest/test_constant_cache.rb diff --git a/benchmark/constant_invalidation.rb b/benchmark/constant_invalidation.rb deleted file mode 100644 index a95ec6f37e..0000000000 --- a/benchmark/constant_invalidation.rb +++ /dev/null @@ -1,22 +0,0 @@ https://github.com/ruby/ruby/blob/trunk/#L0 -$VERBOSE = nil - -CONSTANT1 = 1 -CONSTANT2 = 1 -CONSTANT3 = 1 -CONSTANT4 = 1 -CONSTANT5 = 1 - -def constants - [CONSTANT1, CONSTANT2, CONSTANT3, CONSTANT4, CONSTANT5] -end - -500_000.times do - constants - - # With previous behavior, this would cause all of the constant caches - # associated with the constant lookups listed above to invalidate, meaning - # they would all have to be fetched again. With current behavior, it only - # invalidates when a name matches, so the following constant set shouldn't - # impact the constant lookups listed above. - INVALIDATE = true -end diff --git a/bootstraptest/test_constant_cache.rb b/bootstraptest/test_constant_cache.rb deleted file mode 100644 index 1fa83256ed..0000000000 --- a/bootstraptest/test_constant_cache.rb +++ /dev/null @@ -1,187 +0,0 @@ https://github.com/ruby/ruby/blob/trunk/#L0 -# Constant lookup is cached. -assert_equal '1', %q{ - CONST = 1 - - def const - CONST - end - - const - const -} - -# Invalidate when a constant is set. -assert_equal '2', %q{ - CONST = 1 - - def const - CONST - end - - const - - CONST = 2 - - const -} - -# Invalidate when a constant of the same name is set. -assert_equal '1', %q{ - CONST = 1 - - def const - CONST - end - - const - - class Container - CONST = 2 - end - - const -} - -# Invalidate when a constant is removed. -assert_equal 'missing', %q{ - class Container - CONST = 1 - - def const - CONST - end - - def self.const_missing(name) - 'missing' - end - - new.const - remove_const :CONST - end - - Container.new.const -} - -# Invalidate when a constant's visibility changes. -assert_equal 'missing', %q{ - class Container - CONST = 1 - - def self.const_missing(name) - 'missing' - end - end - - def const - Container::CONST - end - - const - - Container.private_constant :CONST - - const -} - -# Invalidate when a constant's visibility changes even if the call to the -# visibility change method fails. -assert_equal 'missing', %q{ - class Container - CONST1 = 1 - - def self.const_missing(name) - 'missing' - end - end - - def const1 - Container::CONST1 - end - - const1 - - begin - Container.private_constant :CONST1, :CONST2 - rescue NameError - end - - const1 -} - -# Invalidate when a module is included. -assert_equal 'INCLUDE', %q{ - module Include - CONST = :INCLUDE - end - - class Parent - CONST = :PARENT - end - - class Child < Parent - def const - CONST - end - - new.const - - include Include - end - - Child.new.const -} - -# Invalidate when const_missing is hit. -assert_equal '2', %q{ - module Container - Foo = 1 - Bar = 2 - - class << self - attr_accessor :count - - def const_missing(name) - @count += 1 - @count == 1 ? Foo : Bar - end - end - - @count = 0 - end - - def const - Container::Baz - end - - const - const -} - -# Invalidate when the iseq gets cleaned up. -assert_equal '2', %q{ - CONSTANT = 1 - - iseq = RubyVM::InstructionSequence.compile(<<~RUBY) - CONSTANT - RUBY - - iseq.eval - iseq = nil - - GC.start - CONSTANT = 2 -} - -# Invalidate when the iseq gets cleaned up even if it was never in the cache. -assert_equal '2', %q{ - CONSTANT = 1 - - iseq = RubyVM::InstructionSequence.compile(<<~RUBY) - CONSTANT - RUBY - - iseq = nil - - GC.start - CONSTANT = 2 -} diff --git a/class.c b/class.c index 784942a3da..9988b080d9 100644 --- a/class.c +++ b/class.c @@ -1169,20 +1169,11 @@ module_in_super_chain(const VALUE klass, VALUE module) https://github.com/ruby/ruby/blob/trunk/class.c#L1169 return false; } -// For each ID key in the class constant table, we're going to clear the VM's -// inline constant caches associated with it. -static enum rb_id_table_iterator_result -clear_constant_cache_i(ID id, VALUE value, void *data) -{ - rb_clear_constant_cache_for_id(id); - return ID_TABLE_CONTINUE; -} - static int do_include_modules_at(const VALUE klass, VALUE c, VALUE module, int search_super, bool check_cyclic) { VALUE p, iclass, origin_stack = 0; - int method_changed = 0, add_subclass; + int method_changed = 0, constant_changed = 0, add_subclass; long origin_len; VALUE klass_origin = RCLASS_ORIGIN(klass); VALUE original_klass = klass; @@ -1275,12 +1266,13 @@ do_include_modules_at(const VALUE klass, VALUE c, VALUE module, int search_super https://github.com/ruby/ruby/blob/trunk/class.c#L1266 } tbl = RCLASS_CONST_TBL(module); - if (tbl && rb_id_table_size(tbl)) - rb_id_table_foreach(tbl, clear_constant_cache_i, (void *) 0); + if (tbl && rb_id_table_size(tbl)) constant_changed = 1; skip: module = RCLASS_SUPER(module); } + if (constant_changed) rb_clear_constant_cache(); + return method_changed; } diff --git a/include/ruby/backward.h b/include/ruby/backward.h index f5662f13d5..f804c2c36e 100644 --- a/include/ruby/backward.h +++ b/include/ruby/backward.h @@ -15,6 +15,8 @@ https://github.com/ruby/ruby/blob/trunk/include/ruby/backward.h#L15 #define RBIMPL_ATTR_DEPRECATED_INTERNAL(ver) RBIMPL_ATTR_DEPRECATED(("since "#ver", also internal")) #define RBIMPL_ATTR_DEPRECATED_INTERNAL_ONLY() RBIMPL_ATTR_DEPRECATED(("only for internal use")) +RBIMPL_ATTR_DEPRECATED_INTERNAL_ONLY() void rb_clear_constant_cache(void); + /* from version.c */ #if defined(RUBY_SHOW_COPYRIGHT_TO_DIE) && !!(RUBY_SHOW_COPYRIGHT_TO_DIE+0) # error RUBY_SHOW_COPYRIGHT_TO_DIE is deprecated diff --git a/include/ruby/internal/intern/vm.h b/include/ruby/internal/intern/vm.h index 76af796b54..eb53c7a356 100644 --- a/include/ruby/internal/intern/vm.h +++ b/include/ruby/internal/intern/vm.h @@ -252,13 +252,6 @@ void rb_undef_alloc_func(VALUE klass); https://github.com/ruby/ruby/blob/trunk/include/ruby/internal/intern/vm.h#L252 */ rb_alloc_func_t rb_get_alloc_func(VALUE klass); -/** - * Clears the inline constant caches associated with a particular ID. Extension - * libraries should not bother with such things. Just forget about this API (or - * even, the presence of constant caches). - */ -void rb_clear_constant_cache_for_id(ID id); - /** * Resembles `alias`. * diff --git a/insns.def b/insns.def index cdec216cfe..c3b2eb9a97 100644 --- a/insns.def +++ b/insns.def @@ -1028,23 +1028,12 @@ opt_getinlinecache https://github.com/ruby/ruby/blob/trunk/insns.def#L1028 (VALUE val) { struct iseq_inline_constant_cache_entry *ice = ic->entry; - - // If there isn't an entry, then we're going to walk through the ISEQ - // starting at this instruction until we get to the associated - // opt_setinlinecache and associate this inline cache with every getconstant - // listed in between. We're doing this here instead of when the instructions - // are first compiled because it's possible to turn off inline caches and we - // want this to work in either case. - if (!ice) { - vm_ic_compile(GET_CFP(), ic); - } - if (ice && vm_ic_hit_p(ice, GET_EP())) { val = ice->value; JUMP(dst); } else { - val = Qnil; + val = Qnil; } } diff --git a/internal/vm.h b/internal/vm.h index c6c6b2ccc2..b14d5472c4 100644 --- a/internal/vm.h +++ b/internal/vm.h @@ -47,6 +47,7 @@ VALUE rb_obj_is_thread(VALUE obj); https://github.com/ruby/ruby/blob/trunk/internal/vm.h#L47 void rb_vm_mark(void *ptr); void rb_vm_each_stack_value(void *ptr, void (*cb)(VALUE, void*), void *ctx); PUREFUNC(VALUE rb_vm_top_self(void)); +void rb_vm_inc_const_missing_count(void); const void **rb_vm_get_insns_address_table(void); VALUE rb_source_location(int *pline); const char *rb_source_location_cstr(int *pline); diff --git a/iseq.c b/iseq.c index 5d294e9d31..ffbe9859c3 100644 --- a/iseq.c +++ b/iseq.c @@ (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/