Add write barriers when setting Tasks' scope field.#60571
Add write barriers when setting Tasks' scope field.#60571NHDaly wants to merge 1 commit intorelease-1.12from
Conversation
This is needed because the field is a jl_value_t* being set in native code. Presumably this will fix #59483
5d43bf9 to
47e8c0d
Compare
vtjnash
left a comment
There was a problem hiding this comment.
Assigning to the fields of current task does not use a write barrier (the gc knows this would be too costly)
all, tasks right? Lines 2362 to 2363 in e8bf6c4 |
|
Are they always trapped in the remset? I thought I was told it was only the current task, since the task itself isn't colored (just the metadata update here) when being stored back |
|
You're right, only the current task on all threads gets queued to the remset: https://github.com/JuliaLang/julia/blob/master/src/gc-stock.c#L2770-L2774 So what does that imply for the semantics of the write barrier? Do we need to wb on context switch if we're switching out the task (plus the one assignment in task creation where the task we're assigning is not the current task). |
|
Yes, it is supposed to have one on context switch too Lines 201 to 204 in e57442f The one in creation is usually either jl_gc_wb_knownold or jl_gc_wb_fresh (a no-op in either case), since the only value we have constructed during bootstrapping by the time we need to allocate a task is |
| jl_atomic_store_relaxed(&t->_isexception, 0); | ||
| // Inherit scope from parent task | ||
| t->scope = ct->scope; | ||
| jl_gc_wb(t, ct->scope); |
There was a problem hiding this comment.
Given discussion then - I think this is the only one that is required.
There was a problem hiding this comment.
Except, the task is fresh and jl_malloc_stack is JL_NOTSAFEPOINT, so I think we're back to mystery.
|
Okay, well, this PR didn't help with our crash anyway, so indeed maybe none of these write barriers are needed. I can close the PR -- but reading the thread above, i'm not left with confidence in whether any of these are needed. Are you guys sure that none of them are needed? If you are sure, please close this PR. Or feel free to take it over however you see fit - i relinquish unto you :) |
Yeah, we're pretty sure. The one that assigns to the newly allocated task could possibly use a |
This is needed because the field is a jl_value_t* being set in native code.
Presumably this will fix #59483