Skip to content

Commit

Permalink
give finalizers their own RNG state (#45212)
Browse files Browse the repository at this point in the history
fixes #42752
  • Loading branch information
JeffBezanson authored Jul 14, 2022
1 parent 79bba8a commit 85b895b
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 6 deletions.
16 changes: 16 additions & 0 deletions src/gc.c
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,15 @@ static void jl_gc_run_finalizers_in_list(jl_task_t *ct, arraylist_t *list)
ct->sticky = sticky;
}

static uint64_t finalizer_rngState[4];

void jl_rng_split(uint64_t to[4], uint64_t from[4]);

JL_DLLEXPORT void jl_gc_init_finalizer_rng_state(void)
{
jl_rng_split(finalizer_rngState, jl_current_task->rngState);
}

static void run_finalizers(jl_task_t *ct)
{
// Racy fast path:
Expand All @@ -396,9 +405,16 @@ static void run_finalizers(jl_task_t *ct)
}
jl_atomic_store_relaxed(&jl_gc_have_pending_finalizers, 0);
arraylist_new(&to_finalize, 0);

uint64_t save_rngState[4];
memcpy(&save_rngState[0], &ct->rngState[0], sizeof(save_rngState));
jl_rng_split(ct->rngState, finalizer_rngState);

// This releases the finalizers lock.
jl_gc_run_finalizers_in_list(ct, &copied_list);
arraylist_free(&copied_list);

memcpy(&ct->rngState[0], &save_rngState[0], sizeof(save_rngState));
}

JL_DLLEXPORT void jl_gc_run_pending_finalizers(jl_task_t *ct)
Expand Down
12 changes: 6 additions & 6 deletions src/task.c
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,7 @@ uint64_t jl_genrandom(uint64_t rngState[4]) JL_NOTSAFEPOINT
return res;
}

static void rng_split(jl_task_t *from, jl_task_t *to) JL_NOTSAFEPOINT
void jl_rng_split(uint64_t to[4], uint64_t from[4]) JL_NOTSAFEPOINT
{
/* TODO: consider a less ad-hoc construction
Ideally we could just use the output of the random stream to seed the initial
Expand All @@ -747,10 +747,10 @@ static void rng_split(jl_task_t *from, jl_task_t *to) JL_NOTSAFEPOINT
0x3688cf5d48899fa7 == hash(UInt(3))|0x01
0x867b4bb4c42e5661 == hash(UInt(4))|0x01
*/
to->rngState[0] = 0x02011ce34bce797f * jl_genrandom(from->rngState);
to->rngState[1] = 0x5a94851fb48a6e05 * jl_genrandom(from->rngState);
to->rngState[2] = 0x3688cf5d48899fa7 * jl_genrandom(from->rngState);
to->rngState[3] = 0x867b4bb4c42e5661 * jl_genrandom(from->rngState);
to[0] = 0x02011ce34bce797f * jl_genrandom(from);
to[1] = 0x5a94851fb48a6e05 * jl_genrandom(from);
to[2] = 0x3688cf5d48899fa7 * jl_genrandom(from);
to[3] = 0x867b4bb4c42e5661 * jl_genrandom(from);
}

JL_DLLEXPORT jl_task_t *jl_new_task(jl_function_t *start, jl_value_t *completion_future, size_t ssize)
Expand Down Expand Up @@ -790,7 +790,7 @@ JL_DLLEXPORT jl_task_t *jl_new_task(jl_function_t *start, jl_value_t *completion
// Inherit logger state from parent task
t->logstate = ct->logstate;
// Fork task-local random state from parent
rng_split(ct, t);
jl_rng_split(t->rngState, ct->rngState);
// there is no active exception handler available on this stack yet
t->eh = NULL;
t->sticky = 1;
Expand Down
1 change: 1 addition & 0 deletions stdlib/Random/src/RNGs.jl
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ end

function __init__()
seed!(GLOBAL_RNG)
ccall(:jl_gc_init_finalizer_rng_state, Cvoid, ())
end


Expand Down
23 changes: 23 additions & 0 deletions stdlib/Random/test/runtests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -994,3 +994,26 @@ end
@test minimum(m) >= 0.094
@test maximum(m) <= 0.106
end

# issue #42752
# test that running finalizers that launch tasks doesn't change RNG stream
function f42752(do_gc::Bool, cell = (()->Any[[]])())
a = rand()
if do_gc
finalizer(cell[1]) do _
@async nothing
end
cell[1] = nothing
GC.gc()
end
b = rand()
(a, b)
end
guardseed() do
for _ in 1:4
Random.seed!(1)
val = f42752(false)
Random.seed!(1)
@test f42752(true) === val
end
end

2 comments on commit 85b895b

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