store bindings now in an array for safe, fast iteration (#48212)

This lets us query and iterate the bindings without needing to hold
locks over the queries, making those operations more scalable and safe
to use across safepoints or concurrently. It is similar to how we
already deal with specializations and datatype caches.

Updates the smallintcache to additionally be more threadsafe for users,
with explicit acquire and release operations.

Author: vtjnash

src/gc-heap-snapshot.cpp

@@ -392,17 +392,18 @@ void _gc_heap_snapshot_record_object_edge(jl_value_t *from, jl_value_t *to, void
                     g_snapshot->names.find_or_create_string_id(path));
 }
 
-void _gc_heap_snapshot_record_module_to_binding(jl_module_t *module, jl_sym_t *name, jl_binding_t *binding) JL_NOTSAFEPOINT
+void _gc_heap_snapshot_record_module_to_binding(jl_module_t *module, jl_binding_t *binding) JL_NOTSAFEPOINT
 {
+    jl_globalref_t *globalref = binding->globalref;
+    jl_sym_t *name = globalref->name;
     auto from_node_idx = record_node_to_gc_snapshot((jl_value_t*)module);
     auto to_node_idx = record_pointer_to_gc_snapshot(binding, sizeof(jl_binding_t), jl_symbol_name(name));
 
     jl_value_t *value = jl_atomic_load_relaxed(&binding->value);
     auto value_idx = value ? record_node_to_gc_snapshot(value) : 0;
     jl_value_t *ty = jl_atomic_load_relaxed(&binding->ty);
     auto ty_idx = ty ? record_node_to_gc_snapshot(ty) : 0;
-    jl_value_t *globalref = (jl_value_t*)binding->globalref;
-    auto globalref_idx = globalref ? record_node_to_gc_snapshot(globalref) : 0;
+    auto globalref_idx = record_node_to_gc_snapshot((jl_value_t*)globalref);
 
     auto &from_node = g_snapshot->nodes[from_node_idx];
     auto &to_node = g_snapshot->nodes[to_node_idx];

src/gc-heap-snapshot.h

@@ -20,7 +20,7 @@ void _gc_heap_snapshot_record_task_to_frame_edge(jl_task_t *from, void *to) JL_N
 void _gc_heap_snapshot_record_frame_to_frame_edge(jl_gcframe_t *from, jl_gcframe_t *to) JL_NOTSAFEPOINT;
 void _gc_heap_snapshot_record_array_edge(jl_value_t *from, jl_value_t *to, size_t index) JL_NOTSAFEPOINT;
 void _gc_heap_snapshot_record_object_edge(jl_value_t *from, jl_value_t *to, void* slot) JL_NOTSAFEPOINT;
-void _gc_heap_snapshot_record_module_to_binding(jl_module_t* module, jl_sym_t *name, jl_binding_t* binding) JL_NOTSAFEPOINT;
+void _gc_heap_snapshot_record_module_to_binding(jl_module_t* module, jl_binding_t* binding) JL_NOTSAFEPOINT;
 // Used for objects managed by GC, but which aren't exposed in the julia object, so have no
 // field or index.  i.e. they're not reachable from julia code, but we _will_ hit them in
 // the GC mark phase (so we can check their type tag to get the size).
@@ -73,10 +73,10 @@ static inline void gc_heap_snapshot_record_object_edge(jl_value_t *from, jl_valu
     }
 }
 
-static inline void gc_heap_snapshot_record_module_to_binding(jl_module_t* module, jl_sym_t *name, jl_binding_t* binding) JL_NOTSAFEPOINT
+static inline void gc_heap_snapshot_record_module_to_binding(jl_module_t* module, jl_binding_t* binding) JL_NOTSAFEPOINT
 {
     if (__unlikely(gc_heap_snapshot_enabled && prev_sweep_full)) {
-        _gc_heap_snapshot_record_module_to_binding(module, name, binding);
+        _gc_heap_snapshot_record_module_to_binding(module, binding);
     }
 }
 

src/gc.c

@@ -2537,22 +2537,35 @@ module_binding: {
         gc_mark_binding_t *binding = gc_pop_markdata(&sp, gc_mark_binding_t);
         jl_binding_t **begin = binding->begin;
         jl_binding_t **end = binding->end;
-        for (; begin < end; begin += 2) {
+        for (; begin < end; begin++) {
             jl_binding_t *b = *begin;
-            if (b == (jl_binding_t*)HT_NOTFOUND)
+            if (b == (jl_binding_t*)jl_nothing)
                 continue;
             verify_parent1("module", binding->parent, begin, "binding_buff");
             // Record the size used for the box for non-const bindings
-            gc_heap_snapshot_record_module_to_binding(binding->parent, (jl_sym_t*)begin[-1], b);
+            gc_heap_snapshot_record_module_to_binding(binding->parent, b);
             if (gc_try_setmark((jl_value_t*)b, &binding->nptr, &tag, &bits)) {
-                begin += 2;
+                begin++;
                 binding->begin = begin;
                 gc_repush_markdata(&sp, gc_mark_binding_t);
                 new_obj = (jl_value_t*)b;
                 goto mark;
             }
         }
+        binding->begin = begin;
         jl_module_t *m = binding->parent;
+        jl_value_t *bindings = (jl_value_t*)jl_atomic_load_relaxed(&m->bindings);
+        if (gc_try_setmark(bindings, &binding->nptr, &tag, &bits)) {
+            gc_repush_markdata(&sp, gc_mark_binding_t);
+            new_obj = (jl_value_t*)bindings;
+            goto mark;
+        }
+        jl_value_t *bindingkeyset = (jl_value_t*)jl_atomic_load_relaxed(&m->bindingkeyset);
+        if (gc_try_setmark(bindingkeyset, &binding->nptr, &tag, &bits)) {
+            gc_repush_markdata(&sp, gc_mark_binding_t);
+            new_obj = bindingkeyset;
+            goto mark;
+        }
         int scanparent = gc_try_setmark((jl_value_t*)m->parent, &binding->nptr, &tag, &bits);
         size_t nusings = m->usings.len;
         if (nusings) {
@@ -2760,8 +2773,9 @@ mark: {
             else if (foreign_alloc)
                 objprofile_count(vt, bits == GC_OLD_MARKED, sizeof(jl_module_t));
             jl_module_t *m = (jl_module_t*)new_obj;
-            jl_binding_t **table = (jl_binding_t**)m->bindings.table;
-            size_t bsize = m->bindings.size;
+            jl_svec_t *bindings = jl_atomic_load_relaxed(&m->bindings);
+            jl_binding_t **table = (jl_binding_t**)jl_svec_data(bindings);
+            size_t bsize = jl_svec_len(bindings);
             uintptr_t nptr = ((bsize + m->usings.len + 1) << 2) | (bits & GC_OLD);
             gc_mark_binding_t markdata = {m, table + 1, table + bsize, nptr};
             gc_mark_stack_push(&ptls->gc_cache, &sp, gc_mark_laddr(module_binding),

src/gf.c

@@ -172,7 +172,7 @@ static jl_method_instance_t *jl_specializations_get_linfo_(jl_method_t *m JL_PRO
             if (!hv)
                 i -= 1;
             assert(jl_svecref(specializations, i) == jl_nothing);
-            jl_svecset(specializations, i, mi); // jl_atomic_store_release?
+            jl_svecset(specializations, i, mi); // jl_atomic_store_relaxed?
             if (hv) {
                 // TODO: fuse lookup and insert steps?
                 jl_smallintset_insert(&m->speckeyset, (jl_value_t*)m, speccache_hash, i, specializations);
@@ -471,39 +471,38 @@ int foreach_mtable_in_module(
         int (*visit)(jl_methtable_t *mt, void *env),
         void *env)
 {
-    size_t i;
-    void **table = m->bindings.table;
-    for (i = 0; i < m->bindings.size; i += 2) {
-        if (table[i+1] != HT_NOTFOUND) {
-            jl_sym_t *name = (jl_sym_t*)table[i];
-            jl_binding_t *b = (jl_binding_t*)table[i+1];
-            JL_GC_PROMISE_ROOTED(b);
-            if (jl_atomic_load_relaxed(&b->owner) == b && b->constp) {
-                jl_value_t *v = jl_atomic_load_relaxed(&b->value);
-                if (v) {
-                    jl_value_t *uw = jl_unwrap_unionall(v);
-                    if (jl_is_datatype(uw)) {
-                        jl_typename_t *tn = ((jl_datatype_t*)uw)->name;
-                        if (tn->module == m && tn->name == name && tn->wrapper == v) {
-                            // this is the original/primary binding for the type (name/wrapper)
-                            jl_methtable_t *mt = tn->mt;
-                            if (mt != NULL && (jl_value_t*)mt != jl_nothing && mt != jl_type_type_mt && mt != jl_nonfunction_mt) {
-                                if (!visit(mt, env))
-                                    return 0;
-                            }
-                        }
-                    }
-                    else if (jl_is_module(v)) {
-                        jl_module_t *child = (jl_module_t*)v;
-                        if (child != m && child->parent == m && child->name == name) {
-                            // this is the original/primary binding for the submodule
-                            if (!foreach_mtable_in_module(child, visit, env))
+    jl_svec_t *table = jl_atomic_load_relaxed(&m->bindings);
+    for (size_t i = 0; i < jl_svec_len(table); i++) {
+        jl_binding_t *b = (jl_binding_t*)jl_svec_ref(table, i);
+        if ((void*)b == jl_nothing)
+            break;
+        jl_sym_t *name = b->globalref->name;
+        if (jl_atomic_load_relaxed(&b->owner) == b && b->constp) {
+            jl_value_t *v = jl_atomic_load_relaxed(&b->value);
+            if (v) {
+                jl_value_t *uw = jl_unwrap_unionall(v);
+                if (jl_is_datatype(uw)) {
+                    jl_typename_t *tn = ((jl_datatype_t*)uw)->name;
+                    if (tn->module == m && tn->name == name && tn->wrapper == v) {
+                        // this is the original/primary binding for the type (name/wrapper)
+                        jl_methtable_t *mt = tn->mt;
+                        if (mt != NULL && (jl_value_t*)mt != jl_nothing && mt != jl_type_type_mt && mt != jl_nonfunction_mt) {
+                            if (!visit(mt, env))
                                 return 0;
                         }
                     }
                 }
+                else if (jl_is_module(v)) {
+                    jl_module_t *child = (jl_module_t*)v;
+                    if (child != m && child->parent == m && child->name == name) {
+                        // this is the original/primary binding for the submodule
+                        if (!foreach_mtable_in_module(child, visit, env))
+                            return 0;
+                    }
+                }
             }
         }
+        table = jl_atomic_load_relaxed(&m->bindings);
     }
     return 1;
 }

src/init.c

@@ -910,10 +910,10 @@ static void post_boot_hooks(void)
     jl_init_box_caches();
 
     // set module field of primitive types
-    int i;
-    void **table = jl_core_module->bindings.table;
-    for (i = 1; i < jl_core_module->bindings.size; i += 2) {
-        if (table[i] != HT_NOTFOUND) {
+    jl_svec_t *bindings = jl_atomic_load_relaxed(&jl_core_module->bindings);
+    jl_value_t **table = jl_svec_data(bindings);
+    for (size_t i = 0; i < jl_svec_len(bindings); i++) {
+        if (table[i] != jl_nothing) {
             jl_binding_t *b = (jl_binding_t*)table[i];
             jl_value_t *v = jl_atomic_load_relaxed(&b->value);
             if (v) {

src/julia.h

@@ -584,8 +584,9 @@ typedef struct _jl_module_t {
     JL_DATA_TYPE
     jl_sym_t *name;
     struct _jl_module_t *parent;
+    _Atomic(jl_svec_t*) bindings;
+    _Atomic(jl_array_t*) bindingkeyset; // index lookup by name into bindings
     // hidden fields:
-    htable_t bindings;
     arraylist_t usings;  // modules with all bindings potentially imported
     jl_uuid_t build_id;
     jl_uuid_t uuid;
@@ -648,12 +649,12 @@ typedef struct _jl_typemap_level_t {
 // contains the TypeMap for one Type
 typedef struct _jl_methtable_t {
     JL_DATA_TYPE
-    jl_sym_t *name; // sometimes a hack used by serialization to handle kwsorter
+    jl_sym_t *name; // sometimes used for debug printing
     _Atomic(jl_typemap_t*) defs;
     _Atomic(jl_array_t*) leafcache;
     _Atomic(jl_typemap_t*) cache;
     _Atomic(intptr_t) max_args;  // max # of non-vararg arguments in a signature
-    jl_module_t *module; // used for incremental serialization to locate original binding
+    jl_module_t *module; // sometimes used for debug printing
     jl_array_t *backedges; // (sig, caller::MethodInstance) pairs
     jl_mutex_t writelock;
     uint8_t offs;  // 0, or 1 to skip splitting typemap on first (function) argument
@@ -986,9 +987,10 @@ STATIC_INLINE jl_value_t *jl_svecset(
 {
     assert(jl_typeis(t,jl_simplevector_type));
     assert(i < jl_svec_len(t));
-    // TODO: while svec is supposedly immutable, in practice we sometimes publish it first
-    // and set the values lazily. Those users should be using jl_atomic_store_release here.
-    jl_svec_data(t)[i] = (jl_value_t*)x;
+    // while svec is supposedly immutable, in practice we sometimes publish it
+    // first and set the values lazily. Those users occasionally might need to
+    // instead use jl_atomic_store_release here.
+    jl_atomic_store_relaxed((_Atomic(jl_value_t*)*)jl_svec_data(t) + i, (jl_value_t*)x);
     jl_gc_wb(t, x);
     return (jl_value_t*)x;
 }