free more thread state in jl_delete_thread and GC #52198
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JeffBezanson
added
multithreading
vtjnash
approved these changes
Nov 16, 2023
|
fixed a few more things:
I also attempted to move thread-local free pages to the global page pool so they can be reused without that thread around, but I'm not sure I did it right. Help requested on that. @d-netto I also still see about 20kb of RSS growth per thread; not sure where it's coming from. On a thread whose ptls is about to be freed, I see its The goal is to get to ~0 growth for churning threads so long-lived processes are possible. |
vtjnash
approved these changes
Nov 22, 2023
d-netto
reviewed
Nov 22, 2023
JeffBezanson
force-pushed
the
jb/freethreadstate
branch
from
67600e0 to
06cae4b
Compare
|
bump? |
1 similar comment
|
bump? |
|
Might need a rebase. |
JeffBezanson
force-pushed
the
jb/freethreadstate
branch
2 times, most recently
from
fe737c4 to
c71f18a
Compare
JeffBezanson
force-pushed
the
jb/freethreadstate
branch
from
c71f18a to
a125bc2
Compare
oscardssmith
removed
the
merge me
tecosaur
pushed a commit
to tecosaur/julia
that referenced
this pull request
Mar 4, 2024
This prevents most memory growth in workloads that start many foreign threads. In the future, we could do even better by moving pages in the heap of an exited thread (and also maybe pooled stacks) elsewhere so they can be reused, and then also free the TLS object itself.
vtjnash
added a commit
that referenced
this pull request
Jan 2, 2025
Prior to this, especially on macOS, the gc-safepoint here would cause the process to segfault as we had already freed the current_task state. Rearrange this code so that the GC interactions (except for the atomic store to current_task) are all handled before entering GC safe, and then signaling the thread is deleted (via setting current_task = NULL, published by jl_unlock_profile_wr to other threads) is last. ``` ERROR: Exception handler triggered on unmanaged thread. Process 53827 stopped * thread #5, stop reason = EXC_BAD_ACCESS (code=2, address=0x100018008) frame #0: 0x0000000100b74344 libjulia-internal.1.12.0.dylib`jl_delete_thread [inlined] jl_gc_state_set(ptls=0x000000011f8b3200, state='\x02', old_state=<unavailable>) at julia_threads.h:272:9 [opt] 269 assert(old_state != JL_GC_CONCURRENT_COLLECTOR_THREAD); 270 jl_atomic_store_release(&ptls->gc_state, state); 271 if (state == JL_GC_STATE_UNSAFE || old_state == JL_GC_STATE_UNSAFE) -> 272 jl_gc_safepoint_(ptls); 273 return old_state; 274 } 275 STATIC_INLINE int8_t jl_gc_state_save_and_set(jl_ptls_t ptls, Target 0: (julia) stopped. (lldb) up frame #1: 0x0000000100b74320 libjulia-internal.1.12.0.dylib`jl_delete_thread [inlined] jl_gc_state_save_and_set(ptls=0x000000011f8b3200, state='\x02') at julia_threads.h:278:12 [opt] 275 STATIC_INLINE int8_t jl_gc_state_save_and_set(jl_ptls_t ptls, 276 int8_t state) 277 { -> 278 return jl_gc_state_set(ptls, state, jl_atomic_load_relaxed(&ptls->gc_state)); 279 } 280 #ifdef __clang_gcanalyzer__ 281 // these might not be a safepoint (if they are no-op safe=>safe transitions), but we have to assume it could be (statically) (lldb) frame #2: 0x0000000100b7431c libjulia-internal.1.12.0.dylib`jl_delete_thread(value=0x000000011f8b3200) at threading.c:537:11 [opt] 534 ptls->root_task = NULL; 535 jl_free_thread_gc_state(ptls); 536 // then park in safe-region -> 537 (void)jl_gc_safe_enter(ptls); 538 } ``` (test incorporated into #55793) (cherry picked from commit 0d09f3d, resolving conflicts from not having backported #52198)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This prevents most memory growth in workloads that start many foreign threads. In the future, we could do even better by moving pages in the heap of an exited thread (and also maybe pooled stacks) elsewhere so they can be reused, and then also free the TLS object itself.