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

WIP run finalizers on own thread #39529

Closed
wants to merge 1 commit into from

Conversation

miguelraz
Copy link
Contributor

@miguelraz miguelraz commented Feb 5, 2021

This is a horrible, no good, very bad first draft.
It likely has a gajillion errors in logic, spelling, judgement and competence.

... but a first draft was made to tackle #35689.

cc @JeffBezanson

@JeffBezanson JeffBezanson added the multithreading Base.Threads and related functionality label Feb 5, 2021
@maleadt
Copy link
Member

maleadt commented Feb 5, 2021

Does that imply finalizers might be run in a different task than the one in which an object was created?

@yuyichao
Copy link
Contributor

yuyichao commented Feb 5, 2021

Does that imply finalizers might be run in a different task than the one in which an object was created?

That has always been the case.

@miguelraz
Copy link
Contributor Author

I believe so - but I'm definitely not the expert here.
The plan according (yet to be lost to the slack hole in #multithreading) is the following:

Jeff Bezanson 
In threading.c:jl_start_threads, we need to start one more thread with a custom thread function that waits on a condition and then runs a list of finalizers when signaled.
Jeff Bezanson  
Where we currently run finalizers at the end of gc_collect, instead signal the condition.
Jeff Bezanson 
(If julia is started with >1 thread)
...
miguel raz guzmán  
Should it mirror partr.c:jl_threadfun or be spawned right after the loop thread in threading.c:jl_start_threads
Jeff Bezanson
It will be similar to jl_threadfun, since it needs most of the same init calls. I'd put it in gc.c since it's part of the GC.

@vtjnash vtjnash marked this pull request as draft April 16, 2021 18:54
@JeffBezanson JeffBezanson self-requested a review April 29, 2021 16:18
@JeffBezanson JeffBezanson self-assigned this Apr 29, 2021
free(targ);

(void)jl_gc_unsafe_enter(ptls);
jl_finish_task(jl_current_task); // noreturn
Copy link
Member

Choose a reason for hiding this comment

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

Calling this will cause the thread to run the scheduler, and start trying to pull general tasks from the queue to run. We don't want to do that; instead it should run a loop that waits on a condition and then calls run_finalizers.


// signal conditions for finalizer thread
if (!ptls->finalizers_inhibited && ptls->locks.len == 0)
jl_gc_thread_finalizer_wait_cond(ptls);
Copy link
Member

Choose a reason for hiding this comment

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

This should use uv_cond_signal.

uv_thread_create(&uvtid, jl_threadfun, t);
}
else {
uv_thread_create(&uvtid, jl_gc_threadfun_finalizer, t);
Copy link
Member

Choose a reason for hiding this comment

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

Need a declaration of jl_gc_threadfun_finalizer in this file.

@fingolfin
Copy link
Member

WIP PR last worked on in mid 2021. Has conflicts. In the meantime we got support for multiple GC threads -- about which I know little to nothing, but I wonder if there is overlap/conflict?

Anyway, it was also referenced elsewhere in December 2023, so maybe there is merit in keeping this around just in case someone decides to pick up the work again?

@vchuravy
Copy link
Member

We now have a thread that uses concurrent sweeping, so we could use that thread for finalizers as well.
We have an open issue so I don't think this PR needs to stay open.

@vchuravy vchuravy closed this Feb 27, 2024
@miguelraz miguelraz deleted the spawnfinalizers branch February 28, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants