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.

Sign up for GitHub

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

Jump to bottom

julia_gc: replace jl_task_stack_buffer by jl_active_task_stack #5724

Merged
merged 1 commit into from
May 23, 2024
Merged

julia_gc: replace jl_task_stack_buffer by jl_active_task_stack #5724

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/julia_gc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,13 +552,12 @@ static void GapRootScanner(int full)
{
jl_ptls_t ptls = jl_get_ptls_states();
jl_task_t * task = (jl_task_t *)jl_get_current_task();
size_t size;
int tid; // unused

// We figure out the end of the stack from the current task. While
// `stack_bottom` is passed to InitBags(), we cannot use that if
// current_task != root_task.
char * stackend = (char *)jl_task_stack_buffer(task, &size, &tid);
stackend += size;
char *dummy, *stackend;
jl_active_task_stack(task, &dummy, &dummy, &dummy, &stackend);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#if !defined(USE_GAP_INSIDE_JULIA)
// The following test overrides the stackend if the following two
Expand Down
Loading