RFR: 8344050: Shenandoah: Retire GC LABs concurrently [v2]

[Y. Srinivas Ramakrishna]

On Wed, 13 Nov 2024 19:10:12 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Test results show a consistent reduction in the time stopped at `init-update-refs` (as expected). Other metrics and benchmark scores are not significantly affected. This change itself does not completely eliminate the safepoint.
>> 
>> Results comparing `-XX:ShenandoahGCMode=satb` to `shenandoah/master` on `aarch64` (results for generational mode and other platforms are similar):
>> 
>> -51.91% sunflow/shenandoahinitupdaterefs_stopped p=0.00000
>>   Control:      0.020ms (+/-  0.00ms)         70
>>   Test:         0.013ms (+/-  0.00ms)         20
>> 
>> -50.70% luindex/shenandoahinitupdaterefs_stopped p=0.00000
>>   Control:      0.019ms (+/-  0.00ms)         70
>>   Test:         0.013ms (+/-  0.00ms)         20
>> 
>> -50.50% avrora/shenandoahinitupdaterefs_stopped p=0.00000
>>   Control:      0.017ms (+/-  0.00ms)        144
>>   Test:         0.012ms (+/-  0.00ms)         42
>> 
>> -49.79% fop/shenandoahinitupdaterefs_stopped p=0.00000
>>   Control:      0.022ms (+/-  0.00ms)        572
>>   Test:         0.015ms (+/-  0.00ms)        164
>> 
>> -47.99% batik/shenandoahinitupdaterefs_stopped p=0.00000
>>   Control:      0.024ms (+/-  0.01ms)        420
>>   Test:         0.016ms (+/-  0.00ms)        120
>> 
>> -42.54% graphchi/shenandoahinitupdaterefs_stopped p=0.00000
>>   Control:      0.020ms (+/-  0.00ms)         70
>>   Test:         0.014ms (+/-  0.00ms)         20
>> 
>> -42.36% xalan/shenandoahinitupdaterefs_stopped p=0.00000
>>   Control:      0.017ms (+/-  0.00ms)        213
>>   Test:         0.012ms (+/-  0.00ms)         60
>> 
>> -23.63% scimark.lu.large/shenandoahinitupdaterefs_stopped p=0.00000
>>   Control:      0.045ms (+/-  0.00ms)         54
>>   Test:         0.037ms (+/-  0.00ms)         12
>> 
>> -20.66% serial/shenandoahinitupdaterefs_stopped p=0.00000
>>   Control:      0.041ms (+/-  0.00ms)        140
>>   Test:         0.034ms (+/-  0.00ms)         40
>> 
>> -20.31% crypto.aes/shenandoahinitupdaterefs_stopped p=0.00000
>>   Control:      0.041ms (+/-  0.00ms)         69
>>   Test:         0.034ms (+/-  0.01ms)         20
>
> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix builds

Changes look good. 

A couple of nits, not sure if they'll make any real difference though.

I also agree that this should go into 24 after the Genshen JEP PR is merged.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1225:

> 1223:     assert(gclab != nullptr, "GCLAB should be initialized for %s", thread->name());
> 1224:     gclab->retire();
> 1225:     if (_resize && ShenandoahThreadLocalData::gclab_size(thread) > 0) {

Nit: This isn't a change you made, but what's the point of the test? May be just set it to 0 if _resize, and save a load and test.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1229:

> 1227:     }
> 1228: 
> 1229:     if (ShenandoahHeap::heap()->mode()->is_generational()) {

Another nit: Just set this during construction in a field of the closure so you can test it cheaply here.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1237:

> 1235:       //  2. We need to establish a trustworthy UpdateWaterMark value within each old-gen heap region
> 1236:       ShenandoahGenerationalHeap::heap()->retire_plab(plab, thread);
> 1237:       if (_resize && ShenandoahThreadLocalData::plab_size(thread) > 0) {

Same as earlier: may be skip the second conjunct in the test.

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/535#pullrequestreview-2437455680
PR Review Comment: https://git.openjdk.org/shenandoah/pull/535#discussion_r1843067447
PR Review Comment: https://git.openjdk.org/shenandoah/pull/535#discussion_r1843068264
PR Review Comment: https://git.openjdk.org/shenandoah/pull/535#discussion_r1843069407