k0kubun	2018-10-20 16:43:50 +0900 (Sat, 20 Oct 2018)

  New Revision: 65213

  https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=65213

  Log:
    vm_insnhelper.c: never cache getinstancevariable twice
    
    We have several options to ensure there's no race condition between main
    thread and MJIT thead about IC reference:
    
    1) Give up caching ivar for multiple classes (or multiple versions of the
       same class) in the same getinstancevariable (This commit's approach)
    2) Allocate new inline cache every time
    
    Other ideas we could think of couldn't eliminate possibilities of race
    condition.
    In 2, it's memory allocation would be slow and it may trigger JIT
    cancellation frequently. So 1 would be fast for both VM and JIT
    situations.

  Modified files:
    trunk/tool/ruby_vm/views/_mjit_compile_ivar.erb
    trunk/vm.c
    trunk/vm_core.h
    trunk/vm_insnhelper.c
Index: vm.c
===================================================================
--- vm.c	(revision 65212)
+++ vm.c	(revision 65213)
@@ -340,7 +340,7 @@ rb_event_flag_t ruby_vm_event_flags; https://github.com/ruby/ruby/blob/trunk/vm.c#L340
 rb_event_flag_t ruby_vm_event_enabled_flags;
 rb_serial_t ruby_vm_global_method_state = 1;
 rb_serial_t ruby_vm_global_constant_state = 1;
-rb_serial_t ruby_vm_class_serial = 1;
+rb_serial_t ruby_vm_class_serial = RUBY_VM_CLASS_SERIAL_MIN_VALID_VALUE;
 
 static void thread_free(void *ptr);
 
Index: tool/ruby_vm/views/_mjit_compile_ivar.erb
===================================================================
--- tool/ruby_vm/views/_mjit_compile_ivar.erb	(revision 65212)
+++ tool/ruby_vm/views/_mjit_compile_ivar.erb	(revision 65213)
@@ -15,7 +15,7 @@ https://github.com/ruby/ruby/blob/trunk/tool/ruby_vm/views/_mjit_compile_ivar.erb#L15
 % end
 %
 % # compiler: Consider cfp->self as T_OBJECT if ic->ic_serial is set
-    if (ic->ic_serial) {
+    if (ic->ic_serial >= RUBY_VM_CLASS_SERIAL_MIN_VALID_VALUE) {
 % # JIT: optimize away motion of sp and pc. This path does not call rb_warning() and so it's always leaf and not `handles_sp`.
 % # <%= render 'mjit_compile_pc_and_sp', locals: { insn: insn } -%>
 %
Index: vm_core.h
===================================================================
--- vm_core.h	(revision 65212)
+++ vm_core.h	(revision 65213)
@@ -1846,4 +1846,9 @@ void rb_postponed_job_flush(rb_vm_t *vm) https://github.com/ruby/ruby/blob/trunk/vm_core.h#L1846
 
 RUBY_SYMBOL_EXPORT_END
 
+/* special values for ruby_vm_class_serial */
+#define RUBY_VM_CLASS_SERIAL_UNSET 0 /* Not cached yet. Fields in `is_entries` and `cc_entries` start from 0 due to ZALLOC_N. */
+#define RUBY_VM_CLASS_SERIAL_INVALID 1 /* Cache invalidated and never cached again. */
+#define RUBY_VM_CLASS_SERIAL_MIN_VALID_VALUE 2 /* Actual class serials are larger than this value. */
+
 #endif /* RUBY_VM_CORE_H */
Index: vm_insnhelper.c
===================================================================
--- vm_insnhelper.c	(revision 65212)
+++ vm_insnhelper.c	(revision 65213)
@@ -976,12 +976,20 @@ vm_getivar(VALUE obj, ID id, IC ic, stru https://github.com/ruby/ruby/blob/trunk/vm_insnhelper.c#L976
 		    if (index < ROBJECT_NUMIV(obj)) {
 			val = ROBJECT_IVPTR(obj)[index];
 		    }
-		    if (!is_attr) {
-			ic->ic_value.index = index;
-			ic->ic_serial = RCLASS_SERIAL(RBASIC(obj)->klass);
-		    }
-		    else { /* call_info */
-			cc->aux.index = (int)index + 1;
+                    if (is_attr) { /* call_info */
+                        cc->aux.index = (int)index + 1;
+                    }
+                    else { /* getinstancevariable */
+                        if (ic->ic_serial == RUBY_VM_CLASS_SERIAL_UNSET) {
+                            /* set ic_serial only for the first time */
+                            ic->ic_value.index = index;
+                            ic->ic_serial = RCLASS_SERIAL(RBASIC(obj)->klass);
+                        }
+                        else if (ic->ic_serial != RUBY_VM_CLASS_SERIAL_INVALID) {
+                            /* never use cache for another class, to avoid race condition with MJIT worker
+                               and to reduce the number of JIT cancellations by code generated for IC hit. */
+                            ic->ic_serial = RUBY_VM_CLASS_SERIAL_INVALID;
+                        }
 		    }
 		}
 	    }

--
ML: ruby-changes@q...
Info: http://www.atdot.net/~ko1/quickml/