RFR: 8314629: Generational ZGC: Clearing All SoftReferences log line lacks GCId [v2]

[Erik Österlund]

On Mon, 1 Jan 2024 15:48:00 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> Hi all,
>> 
>> This patch adds the GCId to the log of soft reference policy. I move the related code to the start of the old generational GC which needs  to add a parameter `ZDriverRequest` in the method `ZDriverMajor::collect_old`. An alternative, which doesn't need to add a parameter, is only moving the related code to the start of the major GC, concretely the method `ZDriverMajor::gc`. But considering the references only be cleared in old generational GC, I make such decision. Waiting for your opinion on it.
>> 
>> Thanks for taking the time to review.
>> 
>> Best Regards,
>> -- Guoxiong
>
> Guoxiong Li has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Delay the log to reference proccessing phase.
>  - Revert

Maybe some small changes...

src/hotspot/share/gc/z/zReferenceProcessor.cpp line 129:

> 127: 
> 128:   if (clear) {
> 129:     _always_clear_policy = true;

I would prefer if this assignment was moved above the if and that you assign the field directly the value of the clear argument, instead of having two assignments.

src/hotspot/share/gc/z/zReferenceProcessor.hpp line 45:

> 43:   ReferencePolicy*     _soft_reference_policy;
> 44:   // Judge whether the _soft_reference_policy is AlwaysClearPolicy or not,
> 45:   // use this extra field to avoid using RTTI (runtime type information).

This comment confuses me more than it helps. I don't care what the type of the current soft reference policy is, as much as I care about its behaviour. Using RTTI to determine this behaviour based on the type of the current soft ref policy is just one alternative implementation for inspecting the behaviour. I don't think it needs to be mentioned.

src/hotspot/share/gc/z/zReferenceProcessor.hpp line 46:

> 44:   // Judge whether the _soft_reference_policy is AlwaysClearPolicy or not,
> 45:   // use this extra field to avoid using RTTI (runtime type information).
> 46:   bool                 _always_clear_policy;

This name doesn't capture that it has something to do with soft references. I think _clear_all_soft_refs is a better name.

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

Changes requested by eosterlund (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17196#pullrequestreview-1799865811
PR Review Comment: https://git.openjdk.org/jdk/pull/17196#discussion_r1439085425
PR Review Comment: https://git.openjdk.org/jdk/pull/17196#discussion_r1439086045
PR Review Comment: https://git.openjdk.org/jdk/pull/17196#discussion_r1439085644