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

Add a function jl_gc_notify_thread_yield before a thread yields for GC #57297

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions src/gc-interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ JL_DLLEXPORT const char* jl_gc_active_impl(void);
// each GC should implement it but it will most likely not be used by other code in the runtime.
// It still needs to be annotated with JL_DLLEXPORT since it is called from Rust by MMTk.
JL_DLLEXPORT void jl_gc_sweep_stack_pools_and_mtarraylist_buffers(jl_ptls_t ptls) JL_NOTSAFEPOINT;
// Notifies the GC that the given thread is about to yield for a GC. ctx is the ucontext for the thread
// if it is already fetched by the caller, otherwise it is NULL.
JL_DLLEXPORT void jl_gc_notify_thread_yield(jl_ptls_t ptls, void* ctx);
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the term yield here, because it conflicts with the usage of the same term in the task subsystem and has a different meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any suggestion that won't cause confusion in the context?

Copy link
Member

Choose a reason for hiding this comment

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

jl_gc_thread_will_join_collection or sth?


// ========================================================================= //
// Metrics
Expand Down
6 changes: 6 additions & 0 deletions src/gc-mmtk.c
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,8 @@ JL_DLLEXPORT void jl_gc_prepare_to_collect(void)
gc_num.total_time_to_safepoint += duration;

if (!jl_atomic_load_acquire(&jl_gc_disable_counter)) {
// This thread will yield.
jl_gc_notify_thread_yield(ptls, NULL);
JL_LOCK_NOGC(&finalizers_lock); // all the other threads are stopped, so this does not make sense, right? otherwise, failing that, this seems like plausibly a deadlock
#ifndef __clang_gcanalyzer__
mmtk_block_thread_for_gc();
Expand Down Expand Up @@ -323,6 +325,10 @@ JL_DLLEXPORT void jl_gc_prepare_to_collect(void)
errno = last_errno;
}

JL_DLLEXPORT void jl_gc_notify_thread_yield(jl_ptls_t ptls, void* ctx) {
// TODO: Save the context of this thread for conservative scanning.
}

// ========================================================================= //
// GC Statistics
// ========================================================================= //
Expand Down
9 changes: 9 additions & 0 deletions src/gc-stock.c
Original file line number Diff line number Diff line change
Expand Up @@ -3458,6 +3458,11 @@ JL_DLLEXPORT void jl_gc_collect(jl_gc_collection_t collection)
gc_cblist_pre_gc, (collection));

if (!jl_atomic_load_acquire(&jl_gc_disable_counter)) {
// This thread will yield.
// jl_gc_notify_thread_yield does nothing for the stock GC at the point, but it may be non empty in the future,
// and this is a place where we should call jl_gc_notify_thread_yield.
// TODO: This call can be removed if requested.
jl_gc_notify_thread_yield(ptls, NULL);
JL_LOCK_NOGC(&finalizers_lock); // all the other threads are stopped, so this does not make sense, right? otherwise, failing that, this seems like plausibly a deadlock
#ifndef __clang_gcanalyzer__
if (_jl_gc_collect(ptls, collection)) {
Expand Down Expand Up @@ -4080,6 +4085,10 @@ JL_DLLEXPORT const char* jl_gc_active_impl(void) {
return "Built with stock GC";
}

JL_DLLEXPORT void jl_gc_notify_thread_yield(jl_ptls_t ptls, void* ctx) {
// Do nothing before a thread yields
}

#ifdef __cplusplus
}
#endif
2 changes: 2 additions & 0 deletions src/signals-unix.c
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,8 @@ JL_NO_ASAN static void segv_handler(int sig, siginfo_t *info, void *context)
return;
}
if (sig == SIGSEGV && info->si_code == SEGV_ACCERR && jl_addr_is_safepoint((uintptr_t)info->si_addr) && !is_write_fault(context)) {
// TODO: We should do the same for other platforms
jl_gc_notify_thread_yield(ct->ptls, context);
jl_set_gc_and_wait(ct);
// Do not raise sigint on worker thread
if (jl_atomic_load_relaxed(&ct->tid) != 0)
Expand Down