Port code for marking global GC objects (ruby#229)

XrXr · kddnewton · web-flow · commit bc136cd2a3c4 · 2022-03-10T10:15:03.000-05:00
Co-authored-by: Kevin Newton &lt;kddnewton@gmail.com&gt;

Co-authored-by: Kevin Newton &lt;kddnewton@gmail.com&gt;
diff --git a/yjit/bindgen/src/main.rs b/yjit/bindgen/src/main.rs
@@ -81,6 +81,9 @@ fn main() {
         // From include/ruby/internal/core/rclass.h
         .allowlist_function("rb_class_get_superclass")
 
+        // From include/ruby/internal/intern/gc.h
+        .allowlist_function("rb_gc_mark")
+
         // VALUE variables for Ruby class objects
         // From include/ruby/internal/globals.h
         .allowlist_var("rb_cBasicObject")
diff --git a/yjit/src/cruby_bindings.inc.rs b/yjit/src/cruby_bindings.inc.rs
@@ -130,6 +130,9 @@ extern "C" {
 extern "C" {
     pub fn rb_intern(name: *const ::std::os::raw::c_char) -> ID;
 }
+extern "C" {
+    pub fn rb_gc_mark(obj: VALUE);
+}
 extern "C" {
     pub fn rb_obj_is_kind_of(obj: VALUE, klass: VALUE) -> VALUE;
 }
diff --git a/yjit/src/invariants.rs b/yjit/src/invariants.rs
@@ -201,6 +201,43 @@ pub extern "C" fn rb_yjit_constant_state_changed() {
     }
 }
 
+/// Callback for marking GC objects inside [Invariants].
+/// See `struct yjijt_root_struct` in C.
+#[no_mangle]
+pub extern "C" fn rb_yjit_root_mark() {
+    // Comment from C YJIT:
+    //
+    // Why not let the GC move the cme keys in this table?
+    // Because this is basically a compare_by_identity Hash.
+    // If a key moves, we would need to reinsert it into the table so it is rehashed.
+    // That is tricky to do, espcially as it could trigger allocation which could
+    // trigger GC. Not sure if it is okay to trigger GC while the GC is updating
+    // references.
+    //
+    // NOTE(alan): since we are using Rust data structures that don't interact
+    // with the Ruby GC now, it might be feasible to allow movement.
+
+    let invariants = Invariants::get_instance();
+
+    // Mark CME imemos
+    for cme in invariants.cme_validity.keys() {
+        let cme = (*cme) as usize;
+        let cme = VALUE(cme);
+
+        unsafe { rb_gc_mark(cme) };
+    }
+
+    // Mark class and iclass objects
+    for klass in invariants.method_lookup.keys() {
+        // TODO: This is a leak. Unused blocks linger in the table forever, preventing the
+        // callee class they speculate on from being collected.
+        // We could do a bespoke weak reference scheme on classes similar to
+        // the interpreter's call cache. See finalizer for T_CLASS and cc_table_free().
+
+        unsafe { rb_gc_mark(*klass) };
+    }
+}
+
 /*
 static void
 yjit_block_assumptions_free(block_t *block)
diff --git a/yjit_iface.c b/yjit_iface.c
@@ -61,54 +61,9 @@ check_cfunc_dispatch(VALUE receiver, struct rb_callinfo *ci, void *callee, rb_ca
 
 // GC root for interacting with the GC
 struct yjit_root_struct {
-    int unused; // empty structs are not legal in C99
+    bool unused; // empty structs are not legal in C99
 };
 
-// Map klass => id_table[mid, set of blocks]
-// While a block `b` is in the table, b->callee_cme == rb_callable_method_entry(klass, mid).
-// See assume_method_lookup_stable()
-static st_table *method_lookup_dependency;
-
-// For adding to method_lookup_dependency data with st_update
-struct lookup_dependency_insertion {
-    //block_t *block;
-    ID mid;
-};
-
-// Map cme => set of blocks
-// See assume_method_lookup_stable()
-static st_table *cme_validity_dependency;
-
-static int
-mark_and_pin_keys_i(st_data_t k, st_data_t v, st_data_t ignore)
-{
-    rb_gc_mark((VALUE)k);
-
-    return ST_CONTINUE;
-}
-
-// GC callback during mark phase
-static void
-yjit_root_mark(void *ptr)
-{
-    if (method_lookup_dependency) {
-        // TODO: This is a leak. Unused blocks linger in the table forever, preventing the
-        // callee class they speculate on from being collected.
-        // We could do a bespoke weak reference scheme on classes similar to
-        // the interpreter's call cache. See finalizer for T_CLASS and cc_table_free().
-        st_foreach(method_lookup_dependency, mark_and_pin_keys_i, 0);
-    }
-
-    if (cme_validity_dependency) {
-        // Why not let the GC move the cme keys in this table?
-        // Because this is basically a compare_by_identity Hash.
-        // If a key moves, we would need to reinsert it into the table so it is rehashed.
-        // That is tricky to do, espcially as it could trigger allocation which could
-        // trigger GC. Not sure if it is okay to trigger GC while the GC is updating
-        // references.
-        st_foreach(cme_validity_dependency, mark_and_pin_keys_i, 0);
-    }
-}
 
 static void
 yjit_root_free(void *ptr)
@@ -120,20 +75,23 @@ static size_t
 yjit_root_memsize(const void *ptr)
 {
     // Count off-gc-heap allocation size of the dependency table
-    return st_memsize(method_lookup_dependency); // TODO: more accurate accounting
+    return 0; // TODO: more accurate accounting
 }
 
 // GC callback during compaction
 static void
 yjit_root_update_references(void *ptr)
 {
+    // Do nothing since we use rb_gc_mark(), which pins.
 }
 
+void rb_yjit_root_mark(void *ptr); // in Rust
+
 // Custom type for interacting with the GC
 // TODO: make this write barrier protected
 static const rb_data_type_t yjit_root_type = {
     "yjit_root",
-    {yjit_root_mark, yjit_root_free, yjit_root_memsize, yjit_root_update_references},
+    {rb_yjit_root_mark, yjit_root_free, yjit_root_memsize, yjit_root_update_references},
     0, 0, RUBY_TYPED_FREE_IMMEDIATELY
 };
 
@@ -361,12 +319,10 @@ rb_yjit_init(void)
 
     // Call the Rust initialization code
     void rb_yjit_init_rust(void);
-    return rb_yjit_init_rust();
+    rb_yjit_init_rust();
 
-    /*
-    // Initialize the GC hooks
+    // Initialize the GC hooks. Do this second as some code depend on Rust initialization.
     struct yjit_root_struct *root;
     VALUE yjit_root = TypedData_Make_Struct(0, struct yjit_root_struct, &yjit_root_type, root);
     rb_gc_register_mark_object(yjit_root);
-    */
 }

Original file line number	Diff line number	Diff line change
`@@ -130,6 +130,9 @@ extern "C" {`
`130`	`130`	`extern "C" {`
`131`	`131`	`pub fn rb_intern(name: *const ::std::os::raw::c_char) -> ID;`
`132`	`132`	`}`
	`133`	`+extern "C" {`
	`134`	`+ pub fn rb_gc_mark(obj: VALUE);`
	`135`	`+}`
`133`	`136`	`extern "C" {`
`134`	`137`	`pub fn rb_obj_is_kind_of(obj: VALUE, klass: VALUE) -> VALUE;`
`135`	`138`	`}`