add a GC safepoint in Task.wait #41441

vilterp · 2021-07-01T21:30:10Z

This gives GC a chance to run during a task switch,
in addition to being triggered by allocations.

Without this, non-allocating tasks may fail to stop for
a long time, even if they're doing IO or calling yield
explicitly, preventing us from stopping the world for GC.

Fixes #40972 (I think?)

cc @NHDaly @dewilson @vchuravy

This gives GC a chance to run during a task switch, in addition to being triggered by allocations. Without this, non-allocating tasks may fail to stop for a long time, even if they're doing IO or calling `yield` explicitly, preventing us from stopping the world for GC.

vilterp · 2021-07-01T21:31:24Z

base/task.jl

@@ -818,6 +818,7 @@ end
 end

 function wait()
+    GC.safepoint()


Not sure if this is the right place to put this, but it prevents the shell from hanging when we pasted in the simple program in the linked issue.

Let me know if somewhere else in this function, or another function is better!

There is an unconditional safepoint in process_events() after the yield, but I suppose if the code is not returning to an existing Task, we would not reach that?

Oh, interesting, good to know! I see it now, here:

julia/src/jl_uv.c

Line 210 in 5f18285

jl_gc_safepoint_(ct->ptls);

Was that added after Julia 1.6? We were experiencing some long pauses in prod on 1.6, which is what got us looking into this. But seems like it's already been addressed on master?

In any case, as you say, it seems like pausing for GC before task yielding would give slightly more opportunities for GC to run, but i agree that it doesn't seem much different.

NHDaly · 2021-07-01T22:08:28Z

The example problem we ran from #40972 hangs on Julia 1.6.

We tried running it on master, and it seems like it doesnt hang at first, but then it locks my terminal after a few seconds, even to the point where it can't be interrupted with ctrl-z.

After this PR, it seems to work correctly again.

To add some more context, my understanding is that yield() used to be a GC yield point as well, because there were incidental allocations in it's implementation. But as it's been optimized over time, the allocations have been removed, and so it no longer contains any GC yields. This PR attempts to add back an explicitGC yield-point, to Task yield, to prevent the situation where well-tuned, non-allocating code can run for a long time and starve GC.

Since Task yield is a relatively expensive operation compared to gc_safepoint, this seems like a safe place to inject it.

@vtjnash does that seem reasonable to you as well? :) Thanks!

JeffBezanson · 2021-07-07T22:07:19Z

We tried running it on master, and it seems like it doesnt hang at first, but then it locks my terminal after a few seconds, even to the point where it can't be interrupted with ctrl-z.

Interesting; I can reproduce the problem on 1.6 but not this behavior.

JeffBezanson · 2021-07-07T22:58:09Z

This is very cheap though so might as well merge it.

NHDaly

This is very cheap though so might as well merge it.

Oh wow, thanks @JeffBezanson! Sorry for my lack of reply; i was out for the long holiday weekend. I'm leaving some comments inline.

We tried running it on master, and it seems like it doesnt hang at first, but then it locks my terminal after a few seconds, even to the point where it can't be interrupted with ctrl-z.

Interesting; I can reproduce the problem on 1.6 but not this behavior.

Yeah, weird, right? The program @vilterp posted in #40972 (comment) consistently locks my REPL after a few more commands, every time. I had julia built on the following commit: ed4c44f.

I just rebuilt at the commit right before this PR, 480ff81, and it doesn't hang there. So I don't think it was related to the changes in this PR.

But anyway, thanks for the merge!

NHDaly · 2021-07-08T00:02:11Z

base/task.jl

@@ -818,6 +818,7 @@ end
 end

 function wait()
+    GC.safepoint()


Oh, interesting, good to know! I see it now, here:

julia/src/jl_uv.c

Line 210 in 5f18285

jl_gc_safepoint_(ct->ptls);

Was that added after Julia 1.6? We were experiencing some long pauses in prod on 1.6, which is what got us looking into this. But seems like it's already been addressed on master?

In any case, as you say, it seems like pausing for GC before task yielding would give slightly more opportunities for GC to run, but i agree that it doesn't seem much different.

NHDaly · 2021-07-08T00:54:27Z

JeffBezanson added the backport 1.7 label 2 hours ago

Also, unclear if this qualifies as a "bug fix", or whatever the criteria is for backport, but we've backported this to 1.6 in our production builds, and it seemed to apply cleanly, so we could consider backport 1.6 as well if you think that makes sense?

Sacha0 · 2021-07-08T11:47:56Z

Was that added after Julia 1.6? We were experiencing some long pauses in prod on 1.6, which is what got us looking into this. But seems like it's already been addressed on master?

Git blame suggests that the jl_uv.c:210 safepoint was added in the pull request that enabled task migration, i.e. after 1.6 branched but happily before 1.7 branched. Testing on 1.7-beta3 might be worthwhile? :)

NHDaly · 2021-07-08T15:20:53Z

Good suggestion, @Sacha0. Cool, so:

I just checked in v1.7-beta2, and I consistently experienced the same "hangs after a few more entries into the REPL" problem.
But then I updated to v1.7-beta3, and that problem is gone; i tried 5 times and never experienced it.

So whatever it was that fixed it on master, has also fixed it on v1.7-beta3. :) And it indeed wasn't related to this PR after all.

EDIT: And of course, as you pointed out, that gc_safepoint line is already in there in 1.7, so in neither beta2 nor beta3 did I experience the original hang reported in #40972. 👍

(cherry picked from commit 97f817a)

vilterp commented Jul 1, 2021

View reviewed changes

NHDaly added GC Garbage collector multithreading Base.Threads and related functionality labels Jul 1, 2021

JeffBezanson merged commit 97f817a into JuliaLang:master Jul 7, 2021

JeffBezanson added the backport 1.7 label Jul 7, 2021

NHDaly reviewed Jul 8, 2021

View reviewed changes

KristofferC added the backport 1.6 Change should be backported to release-1.6 label Jul 8, 2021

KristofferC pushed a commit that referenced this pull request Jul 12, 2021

add a GC safepoint in Task.wait (#41441)

54a40c7

(cherry picked from commit 97f817a)

KristofferC mentioned this pull request Jul 12, 2021

Backports for julia-1.6.3 #41554

Merged

75 tasks

KristofferC pushed a commit that referenced this pull request Jul 19, 2021

add a GC safepoint in Task.wait (#41441)

e053bb7

(cherry picked from commit 97f817a)

KristofferC pushed a commit that referenced this pull request Jul 19, 2021

add a GC safepoint in Task.wait (#41441)

afad5f6

(cherry picked from commit 97f817a)

KristofferC mentioned this pull request Jul 19, 2021

release-1.7: Backports for 1.7-RC1/1.7-beta4 #41499

Merged

82 tasks

KristofferC removed the backport 1.7 label Aug 3, 2021

KristofferC mentioned this pull request Aug 25, 2021

release-1.7: Backports for 1.7-RC1 #41781

Merged

63 tasks

KristofferC pushed a commit that referenced this pull request Aug 31, 2021

add a GC safepoint in Task.wait (#41441)

2916959

(cherry picked from commit 97f817a)

KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Sep 7, 2021

ancapdev mentioned this pull request Sep 23, 2021

GC hang waiting for threads #42364

Open

This comment has been minimized.

Sign in to view

vchuravy deleted the vilterp/gc-safepoint-in-wait branch September 23, 2021 20:04

staticfloat pushed a commit that referenced this pull request Dec 23, 2022

add a GC safepoint in Task.wait (#41441)

d216f5e

(cherry picked from commit 97f817a)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a GC safepoint in Task.wait #41441

add a GC safepoint in Task.wait #41441

vilterp commented Jul 1, 2021 •

edited

Loading

vilterp Jul 1, 2021 •

edited

Loading

vtjnash Jul 2, 2021

NHDaly Jul 8, 2021

NHDaly commented Jul 1, 2021

JeffBezanson commented Jul 7, 2021

JeffBezanson commented Jul 7, 2021

NHDaly left a comment

NHDaly Jul 8, 2021

NHDaly commented Jul 8, 2021

Sacha0 commented Jul 8, 2021

NHDaly commented Jul 8, 2021 •

edited

Loading

This comment has been minimized.

add a GC safepoint in Task.wait #41441

add a GC safepoint in Task.wait #41441

Conversation

vilterp commented Jul 1, 2021 • edited Loading

vilterp Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

vtjnash Jul 2, 2021

Choose a reason for hiding this comment

NHDaly Jul 8, 2021

Choose a reason for hiding this comment

NHDaly commented Jul 1, 2021

JeffBezanson commented Jul 7, 2021

JeffBezanson commented Jul 7, 2021

NHDaly left a comment

Choose a reason for hiding this comment

NHDaly Jul 8, 2021

Choose a reason for hiding this comment

NHDaly commented Jul 8, 2021

Sacha0 commented Jul 8, 2021

NHDaly commented Jul 8, 2021 • edited Loading

This comment has been minimized.

vilterp commented Jul 1, 2021 •

edited

Loading

vilterp Jul 1, 2021 •

edited

Loading

NHDaly commented Jul 8, 2021 •

edited

Loading