-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Task copying: #4085 #15078
Task copying: #4085 #15078
Conversation
newt | ||
end | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are finish testing, most of these should move to the C file.
if (t->stkbuf){ | ||
newt->ssize = t->ssize; // size of saved piece | ||
newt->bufsz = t->bufsz; | ||
newt->stkbuf = allocb(t->bufsz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing a GC root for the new task and a NULL initialization of the stkbuf before filling it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Julia documentation page says JL_GC_PUSH
is needed when performing jl_...
calls. Since there is jl_...
call inside this function, is GC root still needed? Thanks!
EDIT: sorry - I missed the jl_gc_wb_back
call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is needed on every call that can allocate, all the ones that are exported starts with jl_ which is the reason the doc say it that way. Here allocb allocates and therefore the newly allocated task needs to be rooted and valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed now. Thanks!
This function in |
The jit code effectively generate |
This design makes it easy to track C variables on the stack. Does JIT keep a list of objects/variables (created via JIT code) on the stack? If not, how difficult is it to make JIT track them? EDIT: the documentation of Julia internal can be improved by mentioning these key GC design choices :-) |
No, you don't track all of them.
No. (only in debug_info)
Impossible. |
Since this approach seems to be working well for @yebai's project, would it be possible for us to provide enough hooks that people can do this in their own code, together with dire warnings that the world is likely to end if they do so and don't follow certain very strict rules? |
That doesn't sound like a good reason to add code that is otherwise unused, untested, dangerous, breaks the standard library (when used) and deprecated. |
I agree with that, but if we had this, we would want to test it, e.g. but having the Julia-side code necessary to exercise it in the test suite would satisfy the issue. Maybe it's a bad idea, I just hate to force @yebai to maintain a fork to do this, but maybe that's just the situation. |
Hi, @JeffBezanson here is an updated version of the task copying code I mentioned: https://github.com/TuringLang/Turing.jl/blob/master/deps/task.c It works for Julia 0.3 - 0.6 without issues. Though I haven't tested it against 0.7 -- it should work as long as the COPY_STACK mechanism is not changed. It would be great if Julia can support this feature, even as an optional argument that a user can choose to enable/disable. |
UPDATE: the code contained in this PR has been registered as a standalone Julia package, see https://github.com/TuringLang/Libtask.jl |
It's interesting to me that this works and is useful. That goes against what has been claimed about copying stacks not being feasible or a good idea. It seems like at least some limited form of task copying could be officially supported and tested. |
That's not what I've said. I've said that merging this may prevent us from doing some more aggressive allocation optimizations, which may be far more critical and useful. |
Work as in incompatible with the compiler in a unfixable way? Working as in not crashing in simple test cases is trivial. This is exactly what undefined behavior means and it's the same as all the missing GC root bugs.
So no C code (runtime included) allowed on the stack. No LLVM level allocation optimization allowed. No pass by reference arguments allowed on the call stack. Basically no one on the call stack is allowed to take stack address and pass it around. (edit: Calling functions using these features are fine but all of them should have returned by the time you copy the task). The current system implements and heavily relies on all of the above and that's why I said it's unfixable. |
Right; fully supporting this would require flags to change compiler behavior in various ways. It's possible (as long as you don't interleave julia and C code before copying as Yichao said) but would take a concerted effort to develop and maintain. |
Actually I just realized that it's even worse than this... This is basically a |
I'm not sure that's true --- objects only referenced by locals are still only referenced by the same locals. Can you give an example? |
a = Ref(0)
if fork()
a[] = 1
exit_task()
end
wait_for_the_other_task_to_finish()
a[] For the actual This is what I mean by escaping. The variable cannot be accessed from a different scope but that scope can have multiple executions that need to have shared states. Similar to other return-twice bugs but worse. |
This doesn't implement |
It's not a fork but it introduce a "fork point" that can be externally forked? Or in another word just change the example above to. function f()
t = Ref(0);
while true
produce(t[])
t[] = 1 + t[]
end
end And what should this do? t = Task(f)
consume(t); # produce 0
a = copy(t);
consume(a); # produce 1
consume(t); # produce 1 or 2 ? |
That's a better example. Indeed in this mode I think we'd have to turn off alloc-elim of mutable objects in functions that might yield. |
And by fork I just mean something on the line of |
This PR contains a working implementation of task copying. More specifically:
fork
tasks (aka coroutines):newt = copy(t::Task)
.newt
has an independent stack from the original taskt
, i.e.newt
runs independently from the original taskt
.Here is a simple test script for this feature: