Skip to content

Commit 980347d

Browse files
KenoKristofferC
authored andcommitted
Merge adjacent implicit binding partitions (#57995)
After: ``` julia> convert(Core.Binding, GlobalRef(Base, :Intrinsics)) Binding Base.Intrinsics 617:∞ - implicit `using` resolved to constant Core.Intrinsics 0:616 - undefined binding - guard entry julia> convert(Core.Binding, GlobalRef(Base, :Math)) Binding Base.Math 22128:∞ - constant binding to Base.Math 0:22127 - backdated constant binding to Base.Math ``` There is a bit of trickiness here. In particular, the question is, "when do we check" whether the partition next to the one we currently looked at happens to have the same implicit resolution as our current one. The most obvious answer is that we should do it on access, but in practice that would require essentially scanning back and considering every possible world age state at every lookup. This is undesirable - the lookup is not crazy expensive, but it can add up and most world ages we never touch, so it is also wasteful. This instead implements a different approach where we only perform the resolution for world ages that somebody actually asked about, but can then subsequently merge partitions if we do find that they are identical. The logic for that is a bit involved, since we need to be careful to keep the datastructure valid at every point, but does address the issue. Fixes #57923 (cherry picked from commit 1c3878c)
1 parent b3e1a0c commit 980347d

File tree

7 files changed

+186
-47
lines changed

7 files changed

+186
-47
lines changed

src/jltypes.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3266,8 +3266,10 @@ void jl_init_types(void) JL_GC_DISABLED
32663266
jl_svec(5, jl_any_type,
32673267
jl_ulong_type, jl_ulong_type, jl_any_type/*jl_binding_partition_type*/, jl_ulong_type),
32683268
jl_emptysvec, 0, 1, 0);
3269-
const static uint32_t binding_partition_atomicfields[] = { 0b01101 }; // Set fields 1, 3, 4 as atomic
3269+
const static uint32_t binding_partition_atomicfields[] = { 0b01111 }; // Set fields 1, 2, 3, 4 as atomic
32703270
jl_binding_partition_type->name->atomicfields = binding_partition_atomicfields;
3271+
const static uint32_t binding_partition_constfields[] = { 0x100001 }; // Set fields 1, 5 as constant
3272+
jl_binding_partition_type->name->constfields = binding_partition_constfields;
32713273

32723274
jl_binding_type =
32733275
jl_new_datatype(jl_symbol("Binding"), core, jl_any_type, jl_emptysvec,

src/julia.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -760,7 +760,7 @@ typedef struct JL_ALIGNED_ATTR(8) _jl_binding_partition_t {
760760
* } restriction;
761761
*/
762762
jl_value_t *restriction;
763-
size_t min_world;
763+
_Atomic(size_t) min_world;
764764
_Atomic(size_t) max_world;
765765
_Atomic(struct _jl_binding_partition_t *) next;
766766
size_t kind;
@@ -1955,8 +1955,8 @@ JL_DLLEXPORT jl_sym_t *jl_get_root_symbol(void);
19551955
JL_DLLEXPORT jl_value_t *jl_get_binding_value(jl_binding_t *b JL_PROPAGATES_ROOT);
19561956
JL_DLLEXPORT jl_value_t *jl_get_binding_value_in_world(jl_binding_t *b JL_PROPAGATES_ROOT, size_t world);
19571957
JL_DLLEXPORT jl_value_t *jl_get_binding_value_if_const(jl_binding_t *b JL_PROPAGATES_ROOT);
1958-
JL_DLLEXPORT jl_value_t *jl_get_binding_value_if_resolved(jl_binding_t *b JL_PROPAGATES_ROOT) JL_NOTSAFEPOINT;
1959-
JL_DLLEXPORT jl_value_t *jl_get_binding_value_if_resolved_and_const(jl_binding_t *b JL_PROPAGATES_ROOT) JL_NOTSAFEPOINT;
1958+
JL_DLLEXPORT jl_value_t *jl_get_binding_value_if_resolved_debug_only(jl_binding_t *b JL_PROPAGATES_ROOT) JL_NOTSAFEPOINT;
1959+
JL_DLLEXPORT jl_value_t *jl_get_binding_value_if_latest_resolved_and_const_debug_only(jl_binding_t *b JL_PROPAGATES_ROOT) JL_NOTSAFEPOINT;
19601960
JL_DLLEXPORT jl_value_t *jl_declare_const_gf(jl_module_t *mod, jl_sym_t *name);
19611961
JL_DLLEXPORT jl_method_t *jl_method_def(jl_svec_t *argdata, jl_methtable_t *mt, jl_code_info_t *f, jl_module_t *module);
19621962
JL_DLLEXPORT jl_code_info_t *jl_code_for_staged(jl_method_instance_t *linfo, size_t world, jl_code_instance_t **cache);

src/module.c

Lines changed: 104 additions & 34 deletions
Large diffs are not rendered by default.

src/rtutils.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -579,7 +579,7 @@ JL_DLLEXPORT jl_value_t *jl_stderr_obj(void) JL_NOTSAFEPOINT
579579
if (jl_base_module == NULL)
580580
return NULL;
581581
jl_binding_t *stderr_obj = jl_get_module_binding(jl_base_module, jl_symbol("stderr"), 0);
582-
return stderr_obj ? jl_get_binding_value_if_resolved(stderr_obj) : NULL;
582+
return stderr_obj ? jl_get_binding_value_if_resolved_debug_only(stderr_obj) : NULL;
583583
}
584584

585585
// toys for debugging ---------------------------------------------------------
@@ -674,7 +674,7 @@ static int is_globname_binding(jl_value_t *v, jl_datatype_t *dv) JL_NOTSAFEPOINT
674674
jl_sym_t *globname = dv->name->mt != NULL ? dv->name->mt->name : NULL;
675675
if (globname && dv->name->module) {
676676
jl_binding_t *b = jl_get_module_binding(dv->name->module, globname, 0);
677-
jl_value_t *bv = jl_get_binding_value_if_resolved_and_const(b);
677+
jl_value_t *bv = jl_get_binding_value_if_latest_resolved_and_const_debug_only(b);
678678
// The `||` makes this function work for both function instances and function types.
679679
if (bv && (bv == v || jl_typeof(bv) == v))
680680
return 1;

src/staticdata.c

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1776,7 +1776,7 @@ static void jl_write_values(jl_serializer_state *s) JL_GC_DISABLED
17761776
write_uint(f, 0);
17771777
}
17781778
} else {
1779-
write_uint(f, bpart->min_world);
1779+
write_uint(f, jl_atomic_load_relaxed(&bpart->min_world));
17801780
write_uint(f, max_world);
17811781
}
17821782
write_pointerfield(s, (jl_value_t*)jl_atomic_load_relaxed(&bpart->next));
@@ -3597,7 +3597,7 @@ static int jl_validate_binding_partition(jl_binding_t *b, jl_binding_partition_t
35973597
// and allocate a fresh bpart?
35983598
jl_update_loaded_bpart(b, bpart);
35993599
bpart->kind |= (raw_kind & PARTITION_MASK_FLAG);
3600-
if (bpart->min_world > jl_require_world)
3600+
if (jl_atomic_load_relaxed(&bpart->min_world) > jl_require_world)
36013601
goto invalidated;
36023602
}
36033603
if (!jl_bkind_is_some_explicit_import(kind) && kind != PARTITION_KIND_IMPLICIT_GLOBAL)
@@ -3608,7 +3608,8 @@ static int jl_validate_binding_partition(jl_binding_t *b, jl_binding_partition_t
36083608
jl_binding_partition_t *latest_imported_bpart = jl_atomic_load_relaxed(&imported_binding->partitions);
36093609
if (!latest_imported_bpart)
36103610
return 1;
3611-
if (latest_imported_bpart->min_world <= bpart->min_world) {
3611+
if (jl_atomic_load_relaxed(&latest_imported_bpart->min_world) <=
3612+
jl_atomic_load_relaxed(&bpart->min_world)) {
36123613
add_backedge:
36133614
// Imported binding is still valid
36143615
if ((kind == PARTITION_KIND_EXPLICIT || kind == PARTITION_KIND_IMPORTED) &&
@@ -3619,8 +3620,9 @@ static int jl_validate_binding_partition(jl_binding_t *b, jl_binding_partition_t
36193620
}
36203621
else {
36213622
// Binding partition was invalidated
3622-
assert(bpart->min_world == jl_require_world);
3623-
bpart->min_world = latest_imported_bpart->min_world;
3623+
assert(jl_atomic_load_relaxed(&bpart->min_world) == jl_require_world);
3624+
jl_atomic_store_relaxed(&bpart->min_world,
3625+
jl_atomic_load_relaxed(&latest_imported_bpart->min_world));
36243626
}
36253627
invalidated:
36263628
// We need to go through and re-validate any bindings in the same image that

src/toplevel.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,7 @@ void jl_declare_global(jl_module_t *m, jl_value_t *arg, jl_value_t *set_type, in
309309
goto check_type;
310310
}
311311
check_safe_newbinding(gm, gs);
312-
if (bpart->min_world == new_world) {
312+
if (jl_atomic_load_relaxed(&bpart->min_world) == new_world) {
313313
bpart->kind = new_kind | (bpart->kind & PARTITION_MASK_FLAG);
314314
bpart->restriction = global_type;
315315
if (global_type)
@@ -730,7 +730,7 @@ JL_DLLEXPORT jl_binding_partition_t *jl_declare_constant_val2(
730730
JL_LOCK(&world_counter_lock);
731731
size_t new_world = jl_atomic_load_relaxed(&jl_world_counter) + 1;
732732
jl_binding_partition_t *bpart = jl_declare_constant_val3(b, mod, var, val, constant_kind, new_world);
733-
if (bpart->min_world == new_world)
733+
if (jl_atomic_load_relaxed(&bpart->min_world) == new_world)
734734
jl_atomic_store_release(&jl_world_counter, new_world);
735735
JL_UNLOCK(&world_counter_lock);
736736
return bpart;

test/rebinding.jl

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -318,3 +318,68 @@ module UndefinedTransitions
318318
@test Base.Compiler.is_nothrow(Base.Compiler.decode_effects(ci.ipo_purity_bits))
319319
end
320320
end
321+
322+
# Identical implicit partitions should be merge (#57923)
323+
for binding in (convert(Core.Binding, GlobalRef(Base, :Math)),
324+
convert(Core.Binding, GlobalRef(Base, :Intrinsics)))
325+
# Test that these both only have two partitions
326+
@test isdefined(binding, :partitions)
327+
@test isdefined(binding.partitions, :next)
328+
@test !isdefined(binding.partitions.next, :next)
329+
end
330+
331+
# Test various scenarios for implicit partition merging
332+
module MergeStress
333+
for i = 1:5
334+
@eval module $(Symbol("M$i"))
335+
export x, y
336+
const x = 1
337+
const y = 2
338+
end
339+
end
340+
const before = Base.get_world_counter()
341+
using .M1
342+
const afterM1 = Base.get_world_counter()
343+
using .M2
344+
const afterM2 = Base.get_world_counter()
345+
using .M3
346+
const afterM3 = Base.get_world_counter()
347+
using .M4
348+
const afterM4 = Base.get_world_counter()
349+
using .M5
350+
const afterM5 = Base.get_world_counter()
351+
end
352+
353+
function count_partitions(b::Core.Binding)
354+
n = 0
355+
isdefined(b, :partitions) || return n
356+
bpart = b.partitions
357+
while true
358+
n += 1
359+
isdefined(bpart, :next) || break
360+
bpart = bpart.next
361+
end
362+
return n
363+
end
364+
using Base: invoke_in_world
365+
366+
const xbinding = convert(Core.Binding, GlobalRef(MergeStress, :x))
367+
function access_and_count(point)
368+
invoke_in_world(getglobal(MergeStress, point), getglobal, MergeStress, :x)
369+
count_partitions(xbinding)
370+
end
371+
372+
@test count_partitions(xbinding) == 0
373+
@test access_and_count(:afterM1) == 1
374+
# M2 is the first change to the `usings` table after M1. The partitions
375+
# can and should be merged
376+
@test access_and_count(:afterM2) == 1
377+
378+
# There is a gap between M2 and M5 - the partitions should not be merged
379+
@test access_and_count(:afterM5) == 2
380+
381+
# M4 and M5 are adjacent, these partitions should also be merged (in the opposite direction)
382+
@test access_and_count(:afterM4) == 2
383+
384+
# M3 connects all, so we should have a single partition
385+
@test access_and_count(:afterM3) == 1

0 commit comments

Comments
 (0)