ruby-changes:71503
From: Kevin <ko1@a...>
Date: Fri, 25 Mar 2022 01:14:54 +0900 (JST)
Subject: [ruby-changes:71503] 629908586b (master): Finer-grained inline constant cache invalidation
https://git.ruby-lang.org/ruby.git/commit/?id=629908586b From 629908586b4bead1103267652f8b96b1083573a8 Mon Sep 17 00:00:00 2001 From: Kevin Newton <kddnewton@g...> Date: Wed, 24 Nov 2021 10:31:23 -0500 Subject: Finer-grained inline constant cache invalidation Current behavior - caches depend on a global counter. All constant mutations cause caches to be invalidated. ```ruby class A B = 1 end def foo A::B # inline cache depends on global counter end foo # populate inline cache foo # hit inline cache C = 1 # global counter increments, all caches are invalidated foo # misses inline cache due to `C = 1` ``` Proposed behavior - caches depend on name components. Only constant mutations with corresponding names will invalidate the cache. ```ruby class A B = 1 end def foo A::B # inline cache depends constants named "A" and "B" end foo # populate inline cache foo # hit inline cache C = 1 # caches that depend on the name "C" are invalidated foo # hits inline cache because IC only depends on "A" and "B" ``` Examples of breaking the new cache: ```ruby module C # Breaks `foo` cache because "A" constant is set and the cache in foo depends # on "A" and "B" class A; end end B = 1 ``` We expect the new cache scheme to be invalidated less often because names aren't frequently reused. With the cache being invalidated less, we can rely on its stability more to keep our constant references fast and reduce the need to throw away generated code in YJIT. --- 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 | 85 ++++++++++ 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, 461 insertions(+), 82 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..d3cff5f5a9 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, (void *) 0); 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 f804c2c36e..f5662f13d5 100644 --- a/include/ruby/backward.h +++ b/include/ruby/backward.h @@ -15,8 +15,6 @@ 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 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 ca (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/