Skip to content

Commit

Permalink
Revert "gc: avoid cpu stalls when starting" (#45779)
Browse files Browse the repository at this point in the history
Reverts 6e8c78a from #45561
because it seems to be causing deadlock on CI. I suspect this logic is
missing a uv_cond_broadcast for safe-region transitions of ptls->gc_state.
  • Loading branch information
vtjnash authored Jun 22, 2022
1 parent a2cc0f4 commit 68d62ab
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 28 deletions.
17 changes: 16 additions & 1 deletion src/gc.c
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,22 @@ NOINLINE uintptr_t gc_get_stack_ptr(void)

#define should_timeout() 0

void jl_gc_wait_for_the_world(void);
static void jl_gc_wait_for_the_world(void)
{
if (jl_n_threads > 1)
jl_wake_libuv();
for (int i = 0; i < jl_n_threads; i++) {
jl_ptls_t ptls2 = jl_all_tls_states[i];
// This acquire load pairs with the release stores
// in the signal handler of safepoint so we are sure that
// all the stores on those threads are visible.
// We're currently also using atomic store release in mutator threads
// (in jl_gc_state_set), but we may want to use signals to flush the
// memory operations on those threads lazily instead.
while (!jl_atomic_load_relaxed(&ptls2->gc_state) || !jl_atomic_load_acquire(&ptls2->gc_state))
jl_cpu_pause(); // yield?
}
}

// malloc wrappers, aligned allocation

Expand Down
26 changes: 0 additions & 26 deletions src/safepoint.c
Original file line number Diff line number Diff line change
Expand Up @@ -109,31 +109,6 @@ void jl_safepoint_init(void)
jl_safepoint_pages = addr;
}

void jl_gc_wait_for_the_world(void)
{
assert(jl_n_threads);
if (jl_n_threads > 1)
jl_wake_libuv();
for (int i = 0; i < jl_n_threads; i++) {
jl_ptls_t ptls2 = jl_all_tls_states[i];
// This acquire load pairs with the release stores
// in the signal handler of safepoint so we are sure that
// all the stores on those threads are visible.
// We're currently also using atomic store release in mutator threads
// (in jl_gc_state_set), but we may want to use signals to flush the
// memory operations on those threads lazily instead.
while (!jl_atomic_load_relaxed(&ptls2->gc_state) || !jl_atomic_load_acquire(&ptls2->gc_state)) {
// Use system mutexes rather than spin locking to minimize wasted CPU time
// while we wait for other threads reach a safepoint.
// This is particularly important when run under rr.
uv_mutex_lock(&safepoint_lock);
if (!jl_atomic_load_relaxed(&ptls2->gc_state))
uv_cond_wait(&safepoint_cond, &safepoint_lock);
uv_mutex_unlock(&safepoint_lock);
}
}
}

int jl_safepoint_start_gc(void)
{
if (jl_n_threads == 1) {
Expand Down Expand Up @@ -185,7 +160,6 @@ void jl_safepoint_wait_gc(void)
{
// The thread should have set this is already
assert(jl_atomic_load_relaxed(&jl_current_task->ptls->gc_state) != 0);
uv_cond_broadcast(&safepoint_cond);
// Use normal volatile load in the loop for speed until GC finishes.
// Then use an acquire load to make sure the GC result is visible on this thread.
while (jl_atomic_load_relaxed(&jl_gc_running) || jl_atomic_load_acquire(&jl_gc_running)) {
Expand Down
1 change: 0 additions & 1 deletion src/signals-mach.c
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ static int jl_mach_gc_wait(jl_ptls_t ptls2,
arraylist_push(&suspended_threads, (void*)item);
thread_suspend(thread);
uv_mutex_unlock(&safepoint_lock);
uv_cond_broadcast(&safepoint_cond);
return 1;
}

Expand Down

2 comments on commit 68d62ab

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

Please sign in to comment.