Mark codeinst->inferred as atomic and rescope type inference lock #44968

pchintalapudi · 2022-04-13T16:25:33Z

No description provided.

vtjnash · 2022-04-13T21:30:33Z

You need to also tell Julia that it is atomic (see how we deal with code_instance_constfields, for an example)

base/compiler/typeinfer.jl

vtjnash · 2022-04-13T21:33:17Z

base/compiler/typeinfer.jl

@@ -227,7 +227,11 @@ function finish!(interp::AbstractInterpreter, caller::InferenceResult)
 end

 function _typeinf(interp::AbstractInterpreter, frame::InferenceState)
-    typeinf_nocycle(interp, frame) || return false # frame is now part of a higher cycle
+    ccall(:jl_typeinf_begin, Cvoid, ())


This is still a very wide lock. You should only require it inside cache_result! right now

If the typeinf lock is now scoped to just cache_result! rather than all of type inference, is that sufficient to deprioritize the typeinf lock to be below the codegen lock?

aviatesk · 2022-04-14T05:37:48Z

src/codegen.cpp

@@ -8123,8 +8123,7 @@ void jl_compile_workqueue(
            else {
                // Reinfer the function. The JIT came along and removed the inferred
                // method body. See #34993
-                if (policy != CompilationPolicy::Default &&
-                    codeinst->inferred && codeinst->inferred == jl_nothing) {
+                if (policy != CompilationPolicy::Default && jl_atomic_load_relaxed(&codeinst->inferred) == jl_nothing) {


Extract into a temporary value jl_value_t *inferred = jl_atomic_load_relaxed(&codeinst->inferred) and insert explicit NULL check if (inferred && inferred == jl_nothing) following what we do for the other parts?

I'm not seeing what the explicit NULL check contributes to the if statement. If it is null, it's not going to be == to jl_nothing anyways, and if it's equal to jl_nothing we know it's non-null.

Yeah I agree, I wonder if we want to simplify this pattern found in other parts, e.g.

julia/src/precompile.c

Line 339 in 1f42148

if (inferred && inferred != jl_nothing &&

julia/src/staticdata.c

Line 1710 in 1f42148

if (inferred && inferred != jl_nothing) {

pchintalapudi · 2022-04-14T18:48:41Z

Are there any other fields of jl_code_instance_t that we should mark as atomic?

Current non-atomic fields include:

method definition
rettype
rettype_const
world ranges
purity fields
argument escape information
isspecsig
relocatability

vtjnash · 2022-04-14T20:00:20Z

Are there any other fields of jl_code_instance_t that we should mark as atomic?

method definition -- constant
rettype -- constant
rettype_const -- constant
world ranges -- constant
purity fields -- ??
argument escape information -- ??
isspecsig -- protected by codegen_lock (only used by the JIT workqueue)
relocatability -- likely protected by the method write-lock (only used during serialization)

pchintalapudi · 2022-04-14T20:25:17Z

purity fields -- ??
argument escape information -- ??

I think I'll mark these as atomic then, we can always downgrade from atomic to not-atomic if their safety is proven at a later point.

isspecsig -- protected by codegen_lock (only used by the JIT workqueue)

I'll go ahead and make this field as atomic as well, since our plans involve removing the codegen lock too.

Should I add the first few constant fields to the code_instance_constfields array as well?

pchintalapudi · 2022-04-14T20:33:44Z

purity fields -- ??
argument escape information -- ??

I think I'll mark these as atomic then, we can always downgrade from atomic to not-atomic if their safety is proven at a later point.

Actually scanning through the code, these are only set at construction time, so they should also be constant.

vtjnash · 2022-04-14T20:35:04Z

I'll go ahead and make this field as atomic as well

That would not be legal, since they are not independently atomic, but must be read synchronously with the other fields

aviatesk · 2022-04-15T07:20:56Z

Should I add the first few constant fields to the code_instance_constfields array as well?

That would be great, except that we'd probably like to avoid making purity_bits field as non-constant since it is valid to improve optimizer-locally purity information by additional analysis after CodeInstance construction.

base/compiler/typeinfer.jl

pchintalapudi · 2022-04-16T17:59:38Z

Should I add the first few constant fields to the code_instance_constfields array as well?

That would be great, except that we'd probably like to avoid making purity_bits field as non-constant since it is valid to improve optimizer-locally purity information by additional analysis after CodeInstance construction.

I've marked purity_bits as atomic then, to make it clear that the field may be modified on multiple threads.

pchintalapudi · 2022-04-20T15:08:23Z

base/compiler/typeinfer.jl

@@ -394,12 +395,14 @@ function cache_result!(interp::AbstractInterpreter, result::InferenceResult)
    if !already_inferred
        inferred_result = transform_result_for_cache(interp, linfo, valid_worlds, result.src, result.ipo_effects)
        code_cache(interp)[linfo] = CodeInstance(result, inferred_result, valid_worlds)
+        ccall(:jl_typeinf_begin, Cvoid, ())
        if track_newly_inferred[]
            m = linfo.def
            if isa(m, Method)
                m.module != Core && push!(newly_inferred, linfo)


@vtjnash it looks like the only thing that needs to be protected at this point is this newly_inferred array (and potentially the track_newly_inferred boolean). How should this array be handled? Should it become a custom type known to the compiler so that we can put a lock around the object, or should we switch it to a different threadsafe datatype (e.g. some kind of concurrent set), or should we put a julia lock to protect these variables?

Seems like we should delete it. I have no idea what this is. Sounds like debugging?

Though it was added fairly recently (#43990). @timholy do you want to keep it or can we clean up them?

Judging by its uses in system image creation, it doesn't look like it's safe to remove. For now, I'll just guard it with a jl_mutex_t on the C side and remove the typeinf lock altogether, and we can come up with a better plan for dealing with this array later.

vtjnash · 2022-05-17T09:06:11Z

base/compiler/typeinfer.jl

 const newly_inferred = MethodInstance[]
+const consume = :monotonic


@tkf is this even accurate though? consume ordering implies it does data dependency tracking, which this does not do. What we want is only monotonic+depends as used in the linux kernel (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p0124r7.html)

No, this definition is not correct. I totally agree. The reason why I was suggesting this was that, if the author wants to have the release-consume ordering, it's useful for communicating the intention with other programmers. Otherwise, if a programmer (maybe just me) tries to understand the code purely based on the C++ memory model, it becomes really confusing. The word "consume" is a magic spell telling the reader that it's outside (the well-established subset of) the C++ memory model.

If you want READ_ONCE/WRITE_ONCE, I think using macros with similar names also makes sense. I still haven't followed the Linux-Kernel Memory Model so I don't understand when and how they can be used. But I think I actually prefer this idea at a high level.

Should I then define this as depends instead of consume?

The linux kernel model defines depends as a no-op, and defines that monotonic is sufficient for all of our uses

That's an implementation and not a definition. There is a formal definition of monotonic. It doesn't talk about dependencies.

Also, the implementation has to use monotonic and volatile, IIUC. This is probably not required for how it's used in this patch, though.

@pchintalapudi Actually, this PR is probably not the right place to discuss this. Please feel free to revert my suggestion and use literal :monotonic everywhere. I probably should open this myself sometime later.

The use of volatile is orthogonal and forces forward progress for READ_ONCE, which otherwise is not necessarily specified in the C11 memory model, and is not relevant here. All we exactly care is that the linux memory model defines that monotonic is correct here and sufficient (because it defines that depends is a no-op).

The linux kernel has also, thus far, rejected the C11 memory model as the model definition of monotonic does not match the usual hardware behavior. That is why I suggested we will follow the linux memory model, not C11 here.

we will follow the linux memory model, not C11 here

By "here", do you mean in the implementation of the Julia compiler or in the Julia language? I gave up on complaining when I see LKMM in the compiler and the runtime (in fact, the only point I'm suggesting in this PR is to be explicit for cases when/how it uses LKMM) but I'll still strongly oppose the idea of using LKMM in the Julia language :)

The compiler, runtime, and language are all implemented in the Julia language though 😂

If you pick up a "non-default" memory model like LKMM for a given piece of code, you are not using the language that is typically associated with the syntax of the program you are writing. In this sense, Linux is not written in C. Likewise, I didn't mind if the Julia compiler is written in *.jl files but does not use Julia language. All I'm suggesting here is to clearly mark the places that do not use the Julia language.

(In this comment, I'm assuming that memory ordering semantics is equivalent to C++, of course. If you specify the semantics of @atomic :monotonic ... as something different in the Julia language, it may be possible to use LKMM.)

pchintalapudi · 2022-05-18T13:46:00Z

@nanosoldier runtests(ALL, vs = ":master")

vchuravy · 2022-05-22T21:28:07Z

@nanosoldier runtests(ALL, vs = ":master")

nanosoldier · 2022-05-23T05:20:12Z

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

This coopts half of #44968 to mark jl_code_instance_t fields as either atomic or const. Co-authored-by: Valentin Churavy <[email protected]>

This coopts half of JuliaLang#44968 to mark jl_code_instance_t fields as either atomic or const. Co-authored-by: Valentin Churavy <[email protected]>

pchintalapudi requested a review from vtjnash April 13, 2022 16:25

vtjnash reviewed Apr 13, 2022

View reviewed changes

base/compiler/typeinfer.jl Outdated Show resolved Hide resolved

vtjnash reviewed Apr 13, 2022

View reviewed changes

base/compiler/typeinfer.jl Outdated Show resolved Hide resolved

vtjnash reviewed Apr 13, 2022

View reviewed changes

aviatesk reviewed Apr 14, 2022

View reviewed changes

tkf reviewed Apr 16, 2022

View reviewed changes

base/compiler/typeinfer.jl Outdated Show resolved Hide resolved

pchintalapudi mentioned this pull request Apr 19, 2022

Don't lock around typeinf #44857

Closed

pchintalapudi commented Apr 20, 2022

View reviewed changes

pchintalapudi force-pushed the pc/typeinf-small branch 4 times, most recently from 6a8a343 to bdc6f2f Compare May 13, 2022 17:03

vtjnash reviewed May 17, 2022

View reviewed changes

pchintalapudi added 6 commits May 17, 2022 22:06

Mark codeinst->inferred and codeinst->purity_bits as atomic

5afee95

Access codeinst.inferred via @atomic

95e760c

Rename consume -> depends, fix constfields test

849a547

Fix consume -> depends, move newly_inferred to C

1a163fa

Fix minor bugs

b0be472

Disable type inference lock

8e76caf

pchintalapudi force-pushed the pc/typeinf-small branch from bdc6f2f to 8e76caf Compare May 18, 2022 04:47

pchintalapudi mentioned this pull request Jul 28, 2022

Mark up code instance fields as atomic #46203

Merged

vtjnash pushed a commit that referenced this pull request Aug 11, 2022

Mark code instance fields as const or atomic (#46203)

d94ed88

This coopts half of #44968 to mark jl_code_instance_t fields as either atomic or const. Co-authored-by: Valentin Churavy <[email protected]>

pchintalapudi mentioned this pull request Aug 12, 2022

Shrink typeinf lock to just newly_inferred #46324

Merged

vchuravy closed this Sep 6, 2022

giordano deleted the pc/typeinf-small branch September 12, 2022 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark codeinst->inferred as atomic and rescope type inference lock #44968

Mark codeinst->inferred as atomic and rescope type inference lock #44968

pchintalapudi commented Apr 13, 2022

vtjnash commented Apr 13, 2022

vtjnash Apr 13, 2022

pchintalapudi Apr 14, 2022

aviatesk Apr 14, 2022

pchintalapudi Apr 14, 2022

aviatesk Apr 15, 2022

pchintalapudi commented Apr 14, 2022 •

edited

Loading

vtjnash commented Apr 14, 2022

pchintalapudi commented Apr 14, 2022

pchintalapudi commented Apr 14, 2022

vtjnash commented Apr 14, 2022

aviatesk commented Apr 15, 2022

pchintalapudi commented Apr 16, 2022

pchintalapudi Apr 20, 2022

vtjnash Apr 20, 2022

aviatesk Apr 20, 2022

pchintalapudi May 11, 2022

vtjnash May 17, 2022

tkf May 17, 2022

pchintalapudi May 18, 2022

vtjnash May 19, 2022

tkf May 20, 2022

tkf May 20, 2022

vtjnash May 20, 2022

tkf May 20, 2022

vtjnash May 20, 2022

tkf May 20, 2022 •

edited

Loading

pchintalapudi commented May 18, 2022

vchuravy commented May 22, 2022

nanosoldier commented May 23, 2022

		const newly_inferred = MethodInstance[]
		const consume = :monotonic

Mark codeinst->inferred as atomic and rescope type inference lock #44968

Mark codeinst->inferred as atomic and rescope type inference lock #44968

Conversation

pchintalapudi commented Apr 13, 2022

vtjnash commented Apr 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchintalapudi commented Apr 14, 2022 • edited Loading

vtjnash commented Apr 14, 2022

pchintalapudi commented Apr 14, 2022

pchintalapudi commented Apr 14, 2022

vtjnash commented Apr 14, 2022

aviatesk commented Apr 15, 2022

pchintalapudi commented Apr 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkf May 20, 2022 • edited Loading

Choose a reason for hiding this comment

pchintalapudi commented May 18, 2022

vchuravy commented May 22, 2022

nanosoldier commented May 23, 2022

pchintalapudi commented Apr 14, 2022 •

edited

Loading

tkf May 20, 2022 •

edited

Loading