RFR: 8334147: Shenandoah: Avoid taking lock for free set logging [v6]

[Aleksey Shipilev]

On Thu, 27 Jun 2024 07:37:36 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 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.
>
> Xiaolong Peng has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Make log_status_with_heap_locked public

It looks good to me, with minor nits. I also added "disabled" to the issue synopsis to reflect that we really disabling the lock taking when logging is not requested.

I agree pulling `update_capacity_and_used_at_gc` and `record_whole_heap_examined_timestamp` out of the heap-locked region is sane.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 1139:

> 1137: #ifdef ASSERT
> 1138:    || LogTarget(Debug, gc, free)::is_enabled()
> 1139: #endif

OK, I see it matches what `log_status_with_heap_locked` is already doing. 

For future reference, a shorter variant of this would be:


 if (LogTarget(Info, gc, free)::is_enabled() DEBUG_ONLY(|| LogTarget(Debug, gc, free)::is_enabled()) {


...but this variant is also good.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp line 344:

> 342:   void recycle_trash();
> 343:   void log_status();
> 344:   void log_status_with_heap_locked();

Suggestion: this can be just `log_status_under_lock`. We already have other methods with `*_under_lock`.

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

Marked as reviewed by shade (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19915#pullrequestreview-2144624252
PR Review Comment: https://git.openjdk.org/jdk/pull/19915#discussion_r1656686636
PR Review Comment: https://git.openjdk.org/jdk/pull/19915#discussion_r1656689076