ruby-changes:71598
From: Kevin <ko1@a...>
Date: Sat, 2 Apr 2022 03:48:38 +0900 (JST)
Subject: [ruby-changes:71598] 6068da8937 (master): Finer-grained constant cache invalidation (take 2)
https://git.ruby-lang.org/ruby.git/commit/?id=6068da8937 From 6068da8937d7e4358943f95e7450dae7179a7763 Mon Sep 17 00:00:00 2001 From: Kevin Newton <kddnewton@g...> Date: Thu, 31 Mar 2022 11:04:25 -0400 Subject: Finer-grained constant cache invalidation (take 2) This commit reintroduces finer-grained constant cache invalidation. After 8008fb7 got merged, it was causing issues on token-threaded builds (such as on Windows). The issue was that when you're iterating through instruction sequences and using the translator functions to get back the instruction structs, you're either using `rb_vm_insn_null_translator` or `rb_vm_insn_addr2insn2` depending if it's a direct-threading build. `rb_vm_insn_addr2insn2` does some normalization to always return to you the non-trace version of whatever instruction you're looking at. `rb_vm_insn_null_translator` does not do that normalization. This means that when you're looping through the instructions if you're trying to do an opcode comparison, it can change depending on the type of threading that you're using. This can be very confusing. So, this commit creates a new translator function `rb_vm_insn_normalizing_translator` to always return the non-trace version so that opcode comparisons don't have to worry about different configurations. [Feature #18589] --- benchmark/constant_invalidation.rb | 22 +++ bootstraptest/test_constant_cache.rb | 187 +++++++++++++++++++++ class.c | 16 +- include/ruby/internal/intern/vm.h | 7 + insns.def | 13 +- internal/vm.h | 1 - iseq.c | 114 +++++++++++++ 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 +- 17 files changed, 490 insertions(+), 80 deletions(-) create mode 100644 benchmark/constant_invalidation.rb create mode 100644 bootstraptest/test_constant_cache.rb diff --git a/benchmark/constant_invalidation.rb b/benchmark/constant_invalidation.rb new file mode 100644 index 0000000000..a95ec6f37e --- /dev/null +++ b/benchmark/constant_invalidation.rb @@ -0,0 +1,22 @@ https://github.com/ruby/ruby/blob/trunk/benchmark/constant_invalidation.rb#L1 +$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 new file mode 100644 index 0000000000..1fa83256ed --- /dev/null +++ b/bootstraptest/test_constant_cache.rb @@ -0,0 +1,187 @@ https://github.com/ruby/ruby/blob/trunk/bootstraptest/test_constant_cache.rb#L1 +# 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 9988b080d9..39899d5e05 100644 --- a/class.c +++ b/class.c @@ -1169,11 +1169,20 @@ 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, constant_changed = 0, add_subclass; + int method_changed = 0, add_subclass; long origin_len; VALUE klass_origin = RCLASS_ORIGIN(klass); VALUE original_klass = klass; @@ -1266,13 +1275,12 @@ do_include_modules_at(const VALUE klass, VALUE c, VALUE module, int search_super https://github.com/ruby/ruby/blob/trunk/class.c#L1275 } tbl = RCLASS_CONST_TBL(module); - if (tbl && rb_id_table_size(tbl)) constant_changed = 1; + if (tbl && rb_id_table_size(tbl)) + rb_id_table_foreach(tbl, clear_constant_cache_i, NULL); skip: module = RCLASS_SUPER(module); } - if (constant_changed) rb_clear_constant_cache(); - return method_changed; } diff --git a/include/ruby/internal/intern/vm.h b/include/ruby/internal/intern/vm.h index eb53c7a356..76af796b54 100644 --- a/include/ruby/internal/intern/vm.h +++ b/include/ruby/internal/intern/vm.h @@ -252,6 +252,13 @@ 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 c3b2eb9a97..cdec216cfe 100644 --- a/insns.def +++ b/insns.def @@ -1028,12 +1028,23 @@ 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 b14d5472c4..c6c6b2ccc2 100644 --- a/internal/vm.h +++ b/internal/vm.h @@ -47,7 +47,6 @@ 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 ffbe9859c3..b25d185d1a 100644 --- a/iseq.c +++ b/iseq.c @@ -102,12 +102,77 @@ compile_data (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/