Use WeakRefs for spawned tasks to avoid holding unexpected references #1058
Conversation
Most complete explanation is [here](JuliaLang/julia#40626 (comment)). Also discussed [here](#1057). This PR proposes an alternative solution to `clear_thread_states` where that approach can be problematic (interferring with global thread state, not working as expected in nested spawned tasks, etc.). The previous definition also started unending `Timer` tasks that could build up over time. The approach in this PR is to wrap spawned task closure arguments in `WeakRef` to allow them to be GCed as expected once the tasks are finished.
Codecov ReportBase: 89.91% // Head: 89.78% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1058 +/- ##
==========================================
- Coverage 89.91% 89.78% -0.13%
==========================================
Files 8 8
Lines 2250 2241 -9
==========================================
- Hits 2023 2012 -11
- Misses 227 229 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
src/utils.jl
Outdated
| but with an `_` prefix; this avoids the original input being capture by `Task` closure. So usage is like: | ||
|
|
||
| ```julia | ||
| @weakrefspawn ctx foo begin |
There was a problem hiding this comment.
maybe add the @sync part around it to give a full example of intended use?
src/utils.jl
Outdated
| end | ||
|
|
||
| A macro that wraps a `Threads.@spawn` block with `WeakRef` calls for all `args...` arguments, allowing | ||
| them to be garbage collected once the `Task` has finished running. Must be used within a `@syncpreserve` |
There was a problem hiding this comment.
Thank you for working on this!
Above @sync not @syncpreserve is used. Which is correct?
Also are we sure that the "preserve" part touches only the selected variables (this is relevant, as inside the task GC might have to be run if the taks itself allocates a lot).
|
@bkamins, I've updated this PR to use the As noted in that PR, the |
Most complete explanation is here.
Also discussed here.
This PR proposes an alternative solution to
clear_thread_stateswhere that approach can be problematic (interferring with global thread state, not working as expected in nested spawned tasks, etc.). The previous definition also started unendingTimertasks that could build up over time.The approach in this PR is to wrap spawned task closure arguments in
WeakRefto allow them to be GCed as expected once the tasks are finished.