Allow for :foreigncall to transition to GC safe automatically

[vchuravy]

This has been bouncing around as a idea for a while.
One of the challenges around time-to-safepoint has been Julia code
that is calling libraries.

Since foreign code will not include safepoints we see increased latency
when one thread is running a foreign-call and another wants to trigger GC.

The open design question here is:

• Do we expose this as an option the user must "opt-in", e.g. by using a
  keyword arg to @ccall or a specific calling-convetion.
• Or do we turn this on for all ccall, except for Julia runtime calls.

There is relativly little code outside the Julia runtime that needs to be "GC unsafe",
exception are programs that directly use the Julia C-API. Incidentially jl_adopt_thread
and @cfunction/@ccallable do the right thing and transition to "GC unsafe", regardless
of what state the thread currently is in.

I still need to figure out how to reliably detect Julia runtime calls, but I think we can
switch all other calls to "GC safe". We should also consider optimizations that mark large
regions of code without Julia runtime interactions as "GC safe" in particular numeric
for-loops.

Closes #57057

[gbaraldi]

Since we already have the trampoline stuff, what about adding a safepoint into every call into the julia runtime? Similar to the safepoint in JIT function calls?

[Taaitaaiger]

How would this affect packages that use CxxWrap or jlrs to provide access to C++ and Rust libraries, which call back into into Julia from foreign functions?

[fingolfin]

but I think we can
switch all other calls to "GC safe"

All in Julia itself? Perhaps.

All in all Julia packages? Definitely not! That would be a breaking change.

[kpamnany]

All in all Julia packages? Definitely not! That would be a breaking change.

Can you point at a for-instance @fingolfin?

[gbaraldi]

The not common but existing case is for julia code to call c code that calls julia runtime functions.

[fingolfin]

@kpamnany any package using a JLL linking against libjulia would be a first candidate, that includes all packages using CxxWrap (I count 29 in the general registry). For myself, GAP.jl.

I am not saying this is an impossible change, but only if there is a well thought out migration strategy that is coordinated with all stakeholders. But just saying "it probably won't affect that many packages, let's just do it", without a full analysis and without involving the community, repeats the "main" debacle. Let's not go there.

Overall, I prefer "code that is not as fast as it could be but is correct" over "code that is perhaps a bit faster, or not, but will occasionally crash or produce garbage...

A more viable alternative would be an opt-in solution -- but then of course many packages will miss this opportunity. A pity, but it wouldn't leave us worse than we are right now, new packages could benefit from it.

(If we had something like Rust editions, then for "new" packages this could become the default, I guess...?)

[vchuravy]

@eval function put(msg)
    cmsg = Base.cconvert(Cstring, msg)
    ptr = Base.unsafe_convert(Cstring, cmsg)
    $(Expr(:foreigncall, QuoteNode(:puts), Cint, Core.svec(Cstring), 0, true, QuoteNode(:ccall), :ptr, :cmsg))
end

Now is:

L28:                                              ; preds = %L15
; └
;  @ /home/vchuravy/src/julia2/gc_safe.jl:4 within `put`
  %ptls_field18 = getelementptr inbounds {}**, {}*** %tls_pgcstack, i64 2
  %ptls_load19 = load {}**, {}*** %ptls_field18, align 8
  %12 = bitcast {}** %ptls_load19 to i8*
  %gc_state = getelementptr inbounds i8, i8* %12, i64 25
  %13 = load atomic i8, i8* %gc_state monotonic, align 1
  store atomic i8 2, i8* %gc_state release, align 8
  %14 = call i32 bitcast (void ()* @jlplt_puts_689_got.jit to i32 (i64)*)(i64 %string_ptr) #10
  store atomic i8 %13, i8* %gc_state release, align 8
  %15 = getelementptr inbounds {}*, {}** %ptls_load19, i64 2
  %16 = bitcast {}** %15 to i64**
  %safepoint = load i64*, i64** %16, align 8
  fence syncscope("singlethread") seq_cst
  %17 = load volatile i64, i64* %safepoint, align 8
  fence syncscope("singlethread") seq_cst
  %frame.prev14 = load {}*, {}** %frame.prev, align 8
  %18 = bitcast {}*** %tls_pgcstack to {}**
  store {}* %frame.prev14, {}** %18, align 8
  ret i32 %14

The safepoint emission at that point needs to be cleaner.

@vtjnash that's probably closer to what it needs to be?

[kpamnany]

Note for when this lands: we should redo JuliaLang/MbedTLS.jl#265.

[vchuravy]

@gbaraldi I rebased this and confirmed that:

@eval function put(msg)
    cmsg = Base.cconvert(Cstring, msg)
    ptr = Base.unsafe_convert(Cstring, cmsg)
    $(Expr(:foreigncall, QuoteNode(:puts), Cint, Core.svec(Cstring), 0, true, QuoteNode(:ccall), :ptr, :cmsg))
end

@code_llvm put("1")
  %gc_state = getelementptr inbounds i8, ptr %ptls_load14, i64 25
  %4 = load atomic i8, ptr %gc_state monotonic, align 1
  store atomic i8 2, ptr %gc_state release, align 8
  %5 = call i32 @jlplt_puts_1286_got.jit(i64 %bitcast_coercion2) #14
  store atomic i8 %4, ptr %gc_state release, align 8

[gbaraldi]

This is missing some safepoints no?

STATIC_INLINE int8_t jl_gc_state_set(jl_ptls_t ptls, int8_t state,
                                     int8_t old_state)
{
    assert(old_state != JL_GC_PARALLEL_COLLECTOR_THREAD);
    assert(old_state != JL_GC_CONCURRENT_COLLECTOR_THREAD);
    jl_atomic_store_release(&ptls->gc_state, state);
    if (state == JL_GC_STATE_UNSAFE || old_state == JL_GC_STATE_UNSAFE)
        jl_gc_safepoint_(ptls);
    return old_state;
}

[vchuravy]

I only posted an excerpt:

  %ptls_field13 = getelementptr inbounds i8, ptr %tls_pgcstack, i64 16
  %ptls_load14 = load ptr, ptr %ptls_field13, align 8
  %gc_state = getelementptr inbounds i8, ptr %ptls_load14, i64 25
  %4 = load atomic i8, ptr %gc_state monotonic, align 1
  store atomic i8 2, ptr %gc_state release, align 8
  %5 = call i32 @jlplt_puts_1286_got.jit(i64 %bitcast_coercion2) #14
  store atomic i8 %4, ptr %gc_state release, align 8
  %6 = getelementptr inbounds i8, ptr %ptls_load14, i64 16
  %safepoint = load ptr, ptr %6, align 8
  fence syncscope("singlethread") seq_cst
  %7 = load volatile i64, ptr %safepoint, align 8
  fence syncscope("singlethread") seq_cst

[gbaraldi]

And yeah I was gonna say, the conditionality of the safepoint is probably a bit dumb, even in gc_state_set

[vchuravy]

@gbaraldi I would prefer @ccall gc_safe=true, instead of having to use the @assume_effects macro on top of it.

[giordano]

llvmpasses and windows tests are failing

[KristofferC]

Should probably have been in HISTORY since you want it backported

[giordano]

Addressed by #57679 and #57682

[nsajko]

Applying the new option breaks Base.@assume_effects :effect @ccall ..., see issue #59552.