Skip to content

Make precise semantics of world-age increments in top-level thunks #55145

@Keno

Description

@Keno

Recording some extended discussion with @vtjnash earlier today:

Right now, the semantics of world-age increment during top-level evaluation are ill-defined. It basically happens in the interpreter if codegen reaches here:

ct->world_age = jl_atomic_load_acquire(&jl_world_counter);

That's mostly every statement, but not all top-level evaluation goes through this point, so there can be some suprises.

In codegen, world age increment happens at the beginning of any top-level evaluation and then there is a post-processing pass that increments the world age before any call instruction.

julia/src/codegen.cpp

Lines 8996 to 9001 in 0945b9d

if (toplevel && !ctx.is_opaque_closure) {
LoadInst *world = ctx.builder.CreateAlignedLoad(ctx.types().T_size,
prepare_global_in(jl_Module, jlgetworld_global), ctx.types().alignof_ptr);
world->setOrdering(AtomicOrdering::Acquire);
ctx.builder.CreateAlignedStore(world, world_age_field, ctx.types().alignof_ptr);
}

julia/src/codegen.cpp

Lines 9646 to 9654 in 0945b9d

// we're at toplevel; insert an atomic barrier between every instruction
// TODO: inference is invalid if this has any effect (which it often does)
LoadInst *world = new LoadInst(ctx.types().T_size,
prepare_global_in(jl_Module, jlgetworld_global), Twine(),
/*isVolatile*/false, ctx.types().alignof_ptr, /*insertBefore*/&I);
world->setOrdering(AtomicOrdering::Acquire);
StoreInst *store_world = new StoreInst(world, world_age_field,
/*isVolatile*/false, ctx.types().alignof_ptr, /*insertBefore*/&I);
(void)store_world;

There are problems with this. The first is that inference does not model internal world age increments (and even if did, it would basically just end up giving up, because nothing is guaranteed anymore after a world-age increment). This has occasionally lead to complaints (#24316). We currently hack around this by syntactic detection of statements that affect world-age-partitioned global data structure and prohibiting inference of such statements. As noted in the issue this heuristic is incomplete as the existence of Core.eval implies that not all such modifications are syntactically visible (similar issues apply to concurrency).

A second issue is that #55032 has likely introduced divergent behavior between the interpreter and compiler with respect to observing world-age changes. This is probably not a huge problem, but would still break great to avoid.

At the same time, inference of top-level expressions can be valuable particularly when people run benchmarks or tests in toplevel scope, so we would like to preserve our ability to do so. Additionally, we want inference to always be correct, even when run on top-level code (see also e.g. #55144), so it is unsatisfying for inference to be unsound on some valid inputs, even if we never run it in practice.

We discussed various possible improvements. One well-liked solution was to increment world-ages only at the top of any :toplevel argument with corresponding changes in lowering to split definitions into more such blocks. The advantage of such a setup would be that individual top-level code blocks would always be legal to infer, with the world age effects well guarded by Expr(:toplevel). However, this solution appears unworkable because method and struct definitions are permitted access to local-scope variables that do not cross Expr(:toplevel) boundaries.

After some further discussion, we reached the following proposal:

  1. We stop incrementing the world age for every statement. Instead, world age (at toplevel) is incremented to the latest world age at only the following places:
    a. Entry into toplevel evaluation (i.e. at the beginning of Core.eval)
    b. After any evaluation of :method
    c. On the bindings branch, after any introduction of a new binding
  2. When inference's dataflow encounters a world-age affecting expr, it gives up and returns early, giving imprecise (although as precise as possible given the unkown new world age), but correct results for all toplevel code.

There is some tricky interaction with the assignment semantics to global bindings on the bindings branch. In particular, we don't want global assignment to existing bindings to pessimize inference, but at the same time a binding partition replacement requires a world age increment. Since these operations are not fully syntactically separated, global assignment must gain conditional semantic world-age increments. The problem with this is that, as designed, whether an assignment (at toplevel) updates or replaces a global depends on the partition of said binding in the latest world age, which is not a property accessible to inference. A proposal here is to change the semantics to throw a ConcurrencyViolationError if the binding partition in the executing world is not the binding partition in the latest world. This is a little bit unsatisfying as assignment to outdated binding partitions is well defined in non-toplevel code, but possibly the best we can do. It should also not happen in practice unless there's very racy concurrenct evaluation going on.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions