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

[Aleksey Shipilev]

On Thu, 27 Jun 2024 01:18:06 GMT, Y. Srinivas Ramakrishna <ysr 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 adds a public method ```ShenandoahFreeSet::log_status_with_heap_lock``` which acquire lock when there is need to print logs, also make the ```log_status``` private since not expect it to be called out of ShenandoahFreeSet class.
>> 
>> <s>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. </s>
>> 
>> 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.

@ysramakrishna, please approve and sponsor?

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

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