julia_gc: replace jl_task_stack_buffer by jl_active_task_stack #5724
Conversation
The former has been deprecated since 2020.
fingolfin
added
topic: kernel
release notes: not needed
| char * stackend = (char *)jl_task_stack_buffer(task, &size, &tid); | ||
| stackend += size; | ||
| char *dummy, *stackend; | ||
| jl_active_task_stack(task, &dummy, &dummy, &dummy, &stackend); |
There was a problem hiding this comment.
(I am not that familiar with C.) Does this work as expected passing the same reference multiple times?
From the test changes in JuliaLang/julia@791b194#diff-56f74cdd491aa2348d5c9811ddcaa10d76190925157b55f10ae5caae1339b549 (Introduction of jl_active_task_stack) I would expect the third argument to be the stackend we want, and not the fifth.
There was a problem hiding this comment.
Does this work as expected passing the same reference multiple times?
Yes it does (to know that one has to look at the implementation of jl_active_task_stack which I did).
Regarding 3rd vs. 5th argument: Yes, I made a note to myself that in theory, using the 3rd would be better because it would mean we end up scanning less. But analyzing the code of jl_task_stack_buffer and jl_active_task_stack revealed that actually using the 5th is an exact match for what the old codepath computed.
I may revisit using the 3rd in a future revision, but for now using the 5th is the safer bet, as it minimizes the change in behavior compared to what we had before (and what worked). And while using the 3rd might be an optimization, it also has a risk of introducing a bug (namely if it is actually scanning too little). That said, the two value almost always coincide anyway, only for inactive tasks with copy_stack enabled is there even the possibility for a difference.
The former has been deprecated since 2020.