Skip to content

Commit

Permalink
Lock finalizers lists at exit (#49931)
Browse files Browse the repository at this point in the history
  • Loading branch information
kpamnany authored May 23, 2023
1 parent 1143b8f commit c470dc3
Showing 1 changed file with 5 additions and 0 deletions.
5 changes: 5 additions & 0 deletions src/gc.c
Original file line number Diff line number Diff line change
Expand Up @@ -526,12 +526,17 @@ void jl_gc_run_all_finalizers(jl_task_t *ct)
jl_ptls_t* gc_all_tls_states;
gc_n_threads = jl_atomic_load_acquire(&jl_n_threads);
gc_all_tls_states = jl_atomic_load_relaxed(&jl_all_tls_states);
// this is called from `jl_atexit_hook`; threads could still be running
// so we have to guard the finalizers' lists
JL_LOCK_NOGC(&finalizers_lock);
schedule_all_finalizers(&finalizer_list_marked);
for (int i = 0; i < gc_n_threads; i++) {
jl_ptls_t ptls2 = gc_all_tls_states[i];
if (ptls2 != NULL)
schedule_all_finalizers(&ptls2->finalizers);
}
// unlock here because `run_finalizers` locks this
JL_UNLOCK_NOGC(&finalizers_lock);
gc_n_threads = 0;
gc_all_tls_states = NULL;
run_finalizers(ct);
Expand Down

7 comments on commit c470dc3

@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(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.

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks(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 benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

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

Another day, another regression of "inference". @aviatesk any idea what caused this one either, and did you figure out the source of the previous one?

@aviatesk
Copy link
Member

@aviatesk aviatesk commented on c470dc3 May 25, 2023

Choose a reason for hiding this comment

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

I'm running the benchmark.

and did you figure out the source of the previous one?

No, I still don't understand what the "array" regression means.

@aviatesk
Copy link
Member

Choose a reason for hiding this comment

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

another regression of "inference"

It looks like the "inference" regression is introduced by f44be79 (which I didn't expect because the benchmark performed well when invoked from the pull request).

Please sign in to comment.