RFR: 8340490: Shenandoah: Optimize ShenandoahPacer

[Xiaolong Peng]

On Fri, 20 Sep 2024 09:46:50 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

> I am trying to remember why we even bothered to go into negative budget on this path, and then waited for it to recover. I think it is from here: https://mail.openjdk.org/pipermail/shenandoah-dev/2018-April/005559.html. AFAICS, the intent for that fix was to make sure that unsuccessful pacing claim the budget, which this patch also does. And given it apparently improves performance, I don't mind it going in.
> 
> Comprehension question: the actual improvement comes from waiting in 1ms slices, not from anything else? In retrospect, _it is_ silly to wait until the deadline before attempting to claim the pacing budget.

> src/hotspot/share/gc/shenandoah/shenandoahPacer.cpp line 206:
> 
>> 204:     }
>> 205:     new_val = cur - tax;
>> 206:   } while (Atomic::load(&_budget) == cur &&
> 
> I don't think we need this load, since we have _just_ had another load nearby. This should be enough to resolve the contention issues TTAS pattern tries to avoid.

Thanks, reverted TTAS pattern.

> src/hotspot/share/gc/shenandoah/shenandoahPacer.cpp line 257:
> 
>> 255: 
>> 256:   double start = os::elapsedTime();
>> 257:   while (!claimed) {
> 
> I suggest we common some exit paths by writing the loop like this:
> 
> 
> double start_time = os::elapsedTime();
> while (!claimed && (os::elapsedTime() - start_time) < max_delay) {
>   // We could instead assist GC, but this would suffice for now.
>   wait(1);
>   claimed = claim_for_alloc<false>(words);
> }
> if (!claimed) {
>   // Spent local time budget to wait for enough GC progress.
>   // Force allocating anyway, which may mean we outpace GC,
>   // and start Degenerated GC cycle.
>   claimed = claim_for_alloc<true>(words);
>   assert(claimed, "Should always succeed");
> }
> ShenandoahThreadLocalData::add_paced_time(JavaThread::current(), os::elapsedTime() - start_time);

Thanks, refactored the code along with the change to use os::elapsed_counter(), only need handle the nanos to seconds conversion when calling  ShenandoahThreadLocalData::add_paced_time at the last.

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

PR Comment: https://git.openjdk.org/jdk/pull/21099#issuecomment-2364294440
PR Review Comment: https://git.openjdk.org/jdk/pull/21099#discussion_r1769018442
PR Review Comment: https://git.openjdk.org/jdk/pull/21099#discussion_r1769021455