make GC counters thread-local

[JeffBezanson]

This should fix #31923. I see much less memory growth in alloc-heavy threaded loops.

Also #27173

[Keno]

This seems slightly problematic, because the contention on these variables is likely to be high. Can we do something like keep thread-local counters and every n MB of allocation atomically update the global counters?

[JeffBezanson]

Agreed. Making gc counters thread-local has been on my list. Some of the counters also seem slightly unnecessary, e.g. the number of malloc calls.

[yuyichao]

Yes, the counter has to be thread local. Also, I believe for any reasonable allocation measurement experience, the global one needs to be updated when someone query gc_num.

[JeffBezanson]

for any reasonable allocation measurement experience, the global one needs to be updated when someone query gc_num

Yes, of course. I'm thinking we can return a copy of gc_num with all the per-thread counters added in, just with whatever snapshot the calling thread happens to see. Do you think that's good enough?

[0]

I can't tell if this has already been addressed in the above comments, so apologies for the noise if it has. There seems to be some sort of race condition in this patch. If I run

@time Threads.@threads for _ in 1:Threads.nthreads()
    zeros(10^8)
end

with JULIA_NUM_THREADS=16 on commit 5923179, it usually finishes in about 5 seconds. Occasionally, though, it gets stuck (possibly indefinitely, but at least for several hours) with an arbitrary fraction of the threads at 100% CPU utilization. I can't reproduce this on commit 123ff48 (the parent of 5923179).

In case this is useful, here's a snapshot of where the threads are according to gdb (with OPENBLAS_NUM_THREADS=1 so there are fewer irrelevant threads):

Top of stack	Topmost Julia function	Count
__GI___sigtimedwait	signal_listener	1
__GI_epoll_pwait	jl_task_get_next	1
jl_safepoint_wait_gc	jl_safepoint_wait_gc	10
_mm_pause	jl_safepoint_wait_gc	4
_mm_pause	jl_gc_wait_for_the_world	1

[JeffBezanson]

Thanks for that summary of the backtraces. Very convenient presentation of the info!

[JeffBezanson]

OK, I pushed the thread-local version. One interesting question is what the exact intervals should be between atomic updates of the global counter. I tried to use a different interval on each thread, but perhaps it's better just to pick a constant.

[yuyichao]

I think the best way is to make a guess of the next interval per thread during gc and once that's triggered we can do a sync and decide what to do

[StefanKarpinski]

Random?

[yuyichao]

This function need to use atomic relaxed load.

[JeffBezanson]

Why?

[yuyichao]

Hmm, the line number shows up really weird.... The comment was for combine_thread_gc_counts.

(Actually, to avoid UB, all access of these outside of the collection phase should be atomic.)

[Keno]

jl_atomic_fetch_add should return the resulting value which is the right thing to use for the comparison on the next line.

[JeffBezanson]

I think that's add_fetch? But yes we should use it.

[JeffBezanson]

I think the best way is to make a guess of the next interval per thread during gc and once that's triggered we can do a sync and decide what to do

Does that basically mean: keep a per-thread allocation count and a per-thread interval, and use the amount allocated by a thread since the last collection as its next interval value?

[JeffBezanson]

Note: this needs #32238 first.

[yuyichao]

Does that basically mean: keep a per-thread allocation count and a per-thread interval, and use the amount allocated by a thread since the last collection as its next interval value?

Yes. I think the logic in the allocation should be as simple as possible. I feel like the worst case for this is when the allocation pattern changes a lot between thread but even in that case I think it shouldn't be too bad either. We'll certainly put some limit on it

Another thing is that I think we can limit sync to page change since that's the slower path anyway.

[staticfloat]

In a fascinating turn of events, this PR is the first one to have something caught by the analyzegc run! Unfortunately, this PR was too old to have the new version LLVM that would actually error out on finding an issue, but hopefully this is simple enough:

/buildworker/worker/analyzegc_linux64/build/src/gc.c:1023:5: note: Calling potential safepoint from function annotated JL_NOTSAFEPOINT
    combine_thread_gc_counts(&gc_num);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

Is this a false alarm, or an actual issue?