gc_sweep_page assertion triggering in tests

[vtjnash]

gc                                                (1) |        started at 2023-07-03T19:18:26.371 on pid 398
julia: /cache/build/default-amdci4-3/julialang/julia-master/src/gc.c:1380: gc_sweep_page: Assertion `!prev_sweep_full || pg->prev_nold >= pg->nold' failed.
[7136] signal (6.-6): Aborted
in expression starting at /cache/build/default-amdci4-4/julialang/julia-master/julia-889c2e3d81/share/julia/test/gc/binarytree.jl:54
gsignal at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
abort at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0x7fe2d50c640e)
__assert_fail at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
gc_sweep_page at /cache/build/default-amdci4-3/julialang/julia-master/src/gc.c:1380 [inlined]
gc_sweep_pool_page at /cache/build/default-amdci4-3/julialang/julia-master/src/gc.c:1476 [inlined]
gc_sweep_pool at /cache/build/default-amdci4-3/julialang/julia-master/src/gc.c:1563
_jl_gc_collect at /cache/build/default-amdci4-3/julialang/julia-master/src/gc.c:3260
ijl_gc_collect at /cache/build/default-amdci4-3/julialang/julia-master/src/gc.c:3435
gc at ./gcutils.jl:129 [inlined]
gc at ./gcutils.jl:129

https://buildkite.com/julialang/julia-master/builds/25532#01891c91-c02f-4181-b22c-5962d72d1499
@d-netto

[Keno]

Also https://buildkite.com/julialang/julia-master/builds/25615#01892bf1-a0b8-4173-bfbb-a697847103ba (also with rr trace)

[d-netto]

After building with FORCE_ASSERTIONS=1 I'm able to reproduce it by starting Julia with 8 threads and just calling GC.gc() on an infinite loop.

I was able to reproduce on #50021 and later, but not on the immediately preceding commit.

By looking at this PR, seems like it has introduced a race on the page->nold updates.

My guess is that with the changes from #50021, an item may be marked twice. Which can lead to gc_setmark_pool_ being called twice for the same item, finally leading to page->nold being updated more times than it should, which will trigger the assertion above.

CC: @gbaraldi

[d-netto]

(8 GC threads, to clarify)

[gbaraldi]

Interesting, that PR was made on the assumption that trying to mark the same object twice was a fine, but it seems it's actually not. Which makes me think, are the workstealing queues idempotent? Because if not they are also potential sources of this.

[gbaraldi]

One thought I have here is, how much is correctness important on that number, because incrementing that counter(nold) is actually one of the most expensive parts of the mark loop currently, because as me and @vtjnash fetch_add is possibly expensive because it has seq_cst behaviour between the load/store.

[gbaraldi]

At least from what I've seen, getting it wrong means sweeping one extra page, and that should be safe.

[d-netto]

Which makes me think, are the workstealing queues idempotent?

No.

Making the mark-phase cheaper at the cost of losing a bit of precision in the sweep phase seems to be worth it, so I think that relaxing the assertion for multithreaded marking would make sense.