Redo finer-grained inline constant cache invalidation

benchmark/constant_invalidation.rb

@@ -0,0 +1,22 @@
+$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

bootstraptest/test_constant_cache.rb

@@ -0,0 +1,187 @@
+# 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
+}

class.c

@@ -1169,11 +1169,20 @@ module_in_super_chain(const VALUE klass, VALUE module)
     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
 	}
 
         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;
 }
 

include/ruby/internal/intern/vm.h

@@ -252,6 +252,13 @@ void rb_undef_alloc_func(VALUE klass);
  */
 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`.
  *

insns.def

@@ -1028,12 +1028,23 @@ opt_getinlinecache
 (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;
     }
 }
 

internal/vm.h

@@ -47,7 +47,6 @@ VALUE rb_obj_is_thread(VALUE obj);
 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);