Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix jl_new_task to avoid allocating stack twice #39065

Merged
merged 1 commit into from
Jan 22, 2021

Conversation

fingolfin
Copy link
Contributor

If the user requested dedicated stack of a certain size, then jl_new_task immediately allocates the stack by invoking jl_alloc_fiber.

However, it also later nulls t->stkbuf. As a result, ctx_switch invokes jl_alloc_fiber again to allocate another stack.

An alternate fix (assuming my explanation above is even correct...) might be to not pre-allocate the stack here, and leave that to ctx_switch.

Resolves #36361

@fingolfin fingolfin closed this Jan 5, 2021
@fingolfin fingolfin reopened this Jan 5, 2021
@fingolfin fingolfin force-pushed the mh/jl_new_task-stack-alloc branch 2 times, most recently from 67d48e7 to 4219533 Compare January 7, 2021 23:38
@fingolfin
Copy link
Contributor Author

Can this be merged? The failing tests seem to be spurious; at least I have seen them on plenty other PRs:

[ Info: 
backtrace                          (9) |     5.88 |   0.06 |  1.0 |      79.85 |   915.53
exceptions                         (9) |        started at 2021-01-07T16:34:08.497
spawn                              (5) |         failed at 2021-01-07T16:34:10.185
Error During Test at C:\buildbot\worker-tabularasa\tester_win32\build\share\julia\test\testdefs.jl:21
  Got exception outside of a @test
  LoadError: Failed to connect to cache.julialang.org port 443: Timed out while requesting https://cache.julialang.org/https://frippery.org/files/busybox/busybox.exe

and

REPL                               (4) |         failed at 2021-01-08T00:36:15.070
Error During Test at /buildworker/worker/tester_linuxaarch64/build/share/julia/test/testdefs.jl:21
  Got exception outside of a @test
  LoadError: LoadError: "hard kill repl test"

If the user requested dedicated stack of a certain size, then `jl_new_task`
immediately allocates the stack by invoking `jl_alloc_fiber`.

However, it also later nulls `t->stkbuf`. As a result, `ctx_switch` invokes
`jl_alloc_fiber` *again* to allocate another stack.
@vchuravy vchuravy closed this Jan 22, 2021
@vchuravy vchuravy reopened this Jan 22, 2021
@vchuravy vchuravy added the status:merge me PR is reviewed. Merge when all tests are passing label Jan 22, 2021
@fingolfin
Copy link
Contributor Author

buildbot/tester_linuxaarch64 still fails with the same error:

Error During Test at /buildworker/worker/tester_linuxaarch64/build/share/julia/test/testdefs.jl:21
  Got exception outside of a @test
  LoadError: LoadError: "hard kill repl test"
  Stacktrace:
    [1] try_yieldto(undo::typeof(Base.ensure_rescheduled))
      @ Base ./task.jl:705

The same appears in many, many other PRs. E.g. right now I see it in PRs #39358, #39312, #39287, #39285 (and I didn't go beyond the first page of open PRs)

@fingolfin
Copy link
Contributor Author

Yay, buildbot/tester_linuxaarch64 seems to have passed now

@vchuravy vchuravy merged commit ae53238 into JuliaLang:master Jan 22, 2021
@simeonschaub simeonschaub removed the status:merge me PR is reviewed. Merge when all tests are passing label Apr 3, 2021
@fingolfin fingolfin deleted the mh/jl_new_task-stack-alloc branch July 7, 2021 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weirdness (bug) in kernel function jl_new_task
4 participants