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

[Guoxiong Li]

On Mon, 1 Jan 2024 21:12:22 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>> Guoxiong Li has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Delay the log to reference proccessing phase.
>>  - Revert
>
> 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.

Fixed.

> 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.

Fixed. I remove the comment.

> 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.

Renamed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17196#discussion_r1439142908
PR Review Comment: https://git.openjdk.org/jdk/pull/17196#discussion_r1439142961
PR Review Comment: https://git.openjdk.org/jdk/pull/17196#discussion_r1439142936