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

make jl_active_task_stack more accurate #54923

Closed
wants to merge 1 commit into from
Closed

Conversation

JeffBezanson
Copy link
Member

In most cases this used to return the whole stack buffer; instead look at the context or thread state to get the actual SP.

@JeffBezanson JeffBezanson added multithreading Base.Threads and related functionality GC Garbage collector labels Jun 24, 2024
In most cases this used to return the whole stack buffer; instead look at the
context or thread state to get the actual SP.
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Ohh, very nice :-)

@kpamnany
Copy link
Contributor

kpamnany commented Jul 1, 2024

@JeffBezanson: is this done/working? Merge?

@qinsoon, you probably want to backport this once it is merged.

@JeffBezanson
Copy link
Member Author

We concluded that this probably can't be called from the GC, so maybe we shouldn't do this?

@fingolfin
Copy link
Member

I am confused as to how to parse thus... "can't be called" as in "wouldn't work" or as in "doesn't actually happen" or ... ?

@kpamnany
Copy link
Contributor

@fingolfin: for the moving GC (MMTk), we need to pin pointers that should not be moved in the next collection. We're using a conservative stack scan to figure out what needs to be pinned. To make the scan quicker, we wanted to get rsp rather than scanning the entire stack. The approach in this PR leverages the logic in jl_rec_backtrace(), but that code does other things that probably can't be safely done from a GC thread.

We concluded that this probably can't be called from the GC, so maybe we shouldn't do this?

@qinsoon: have you been able to implement a get_sp()? If so, we can close this PR.

@fingolfin
Copy link
Member

@kpamnany OK so the problem is that the function being modified here is not safe to be called during GC, got it (note that we've been doing conservative stack scanning in Julia since... 2020 or so)

@fingolfin
Copy link
Member

Are there still plans by anyone to work on this in some form? I'd be really interested in this. I am also willing to help out with both development and testing. But it seems you guys already had some discussions "on the side" regarding pitfalls and implementation approaches...?

@qinsoon
Copy link
Contributor

qinsoon commented Sep 4, 2024

I don't know if any one is actively working on this. Probably no.

The issue with the PR so far is that it requires the GC threads that call the function to have a Julia TLS. However, having a Julia TLS does not seem to be a requirement for GC threads in other cases. We can make it a requirement, then there would be no issue for this PR.

One alternative @kpamnany mentioned is that to get the stack pointer value from the saved registers in #55003, which also works around the TLS requirement. I am not working on that either.

@vtjnash
Copy link
Member

vtjnash commented Sep 4, 2024

We concluded this would probably need to be a new field in the Task struct that stores the active extent whenever the GC is running, as often there is no other way to retrieve that information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GC Garbage collector multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants