RFR: 8294775: Shenandoah: reduce contention on _threads_in_evac

[Roman Kennke]

On Wed, 5 Oct 2022 11:10:29 GMT, Nick Gasson <ngasson at openjdk.org> wrote:

> The idea here is to reduce contention on the shared `_threads_in_evac` counter by splitting its state over multiple independent cache lines. Each thread hashes to one particular counter based on its `Thread*`. This helps improve throughput of concurrent evacuation where many Java threads may be attempting to update this counter on the load barrier slow path.
> 
> See this earlier thread for details and SPECjbb results: https://mail.openjdk.org/pipermail/shenandoah-dev/2022-October/017494.html
> 
> Also tested `hotspot_gc_shenandoah` on x86 and AArch64.

Hi Nick,

Thank you, that is a useful change! I verified performance and it does improve both throughput and latency on several machines (not as much as for you - but I also have not thrown so many CPUs at it.. )

I do have a few suggestions.

src/hotspot/share/gc/shenandoah/shenandoahEvacOOMHandler.cpp line 40:

> 38: 
> 39: volatile jint *ShenandoahEvacOOMHandler::threads_in_evac_ptr(Thread* t) {
> 40:   uint64_t key = (uintptr_t)t;

Maybe put that in a separate hash(Thread*) function? Also, is that a particular documented hash-function?(Related: In Lilliput project, I am working on a different identity-hash-code implementation, and part of it will be a hash-implementation to hash arbitrary pointers to 32 or 64 bit hash, currently using murmur3. Maybe this could be reused for here, when it happens?)

src/hotspot/share/gc/shenandoah/shenandoahEvacOOMHandler.cpp line 55:

> 53:   // *and* the counter is zero.
> 54:   while (Atomic::load_acquire(ptr) != OOM_MARKER_MASK) {
> 55:     os::naked_short_sleep(1);

Not sure if SpinPause() may be better here? @shipilev probably knows more.

src/hotspot/share/gc/shenandoah/shenandoahEvacOOMHandler.hpp line 88:

> 86:   static const jint OOM_MARKER_MASK;
> 87: 
> 88:   static constexpr jint EVAC_COUNTER_BUCKETS = 64;

Maybe it'd be useful to not hardwire this? It could be a runtime option, possibly diagnostic (not sure). Many workloads would not even use so many threads...

src/hotspot/share/gc/shenandoah/shenandoahEvacOOMHandler.hpp line 92:

> 90:   shenandoah_padding(0);
> 91:   struct {
> 92:     volatile jint bits;

The bits field needs a comment saying that it combines a counter with an OOM bit. In-fact, it would probably benefit from a little bit of refactoring, make it a class, and move accessors and relevant methods into it, and avoid public access to the field?

-------------

Changes requested by rkennke (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10573