RFR: 8334147: Shenandoah: Avoid taking lock for free set logging

[Y. Srinivas Ramakrishna]

On Wed, 26 Jun 2024 21:57:54 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

> Hi all, 
> This pull request propose a fix for the issue https://bugs.openjdk.org/browse/JDK-8334147
> 
>> There are multiple places in Shenandoah where we take heap lock for potential free set diagnostics. There is no point in taking that lock if we do not report anything. We should at very least take the lock only when logging is actually needed.
> 
> Basically it moves the ShenandoahHeapLocker into ShenandoahFreeSet::log_status and only acquire heap lock when log is actually enabled. Since the lock is not a reentrant lock, re-arrangement of code related to ShenandoahFreeSet::rebuild is needed. 
> 
> The change for ShenandoahFreeSet::rebuild is probably debatable, ShenandoahFreeSet::log_status will acquire lock again after ShenandoahFreeSet::rebuild is executed, hence the metrics/information in log may not be always consistent, but it might be fine. 
> 
> Additional test:
> - [x] `make test TEST=hotspot_gc_shenandoah`
> 
> Test summary
> ==============================
>    TEST                                              TOTAL  PASS  FAIL ERROR   
>    jtreg:test/hotspot/jtreg:hotspot_gc_shenandoah      259   259     0     0   
> ==============================
> TEST SUCCESS
> 
> 
> Best,
> Xiaolong.

This _may be_ cleaner and would largely avoid relocating the calls in some cases like you did; _may be_ ....


void log_status() {
    if (logging enabled) {
       lock heap;
       log_status_work_with_lock();
    }
}


where the (now) private work method `log_status_work_with_lock()` asserts that logging is enabled and lock is held, then does the logging work without checking if logging is enabled like the old `log_status()` did.

For the other callers who call from within contexts where the lock is held, you can have a variant that calls `log_status_with_lock()` which does:


void log_status_with_lock() {
    assert that heap lock is held;
    if (logging enabled) {
      log_status_work_with_lock();
    }
}


Not sure if this is worthwhile though.

Will let others more familiar with Shenandoah code opine/advise on that.

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

PR Comment: https://git.openjdk.org/jdk/pull/19915#issuecomment-2192876959