RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]

[William Kemper]

On Mon, 18 Nov 2024 16:20:09 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> William Kemper has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 510 commits:
>> 
>>  - Merge branch 'merge-latest' into great-genshen-pr-redux
>>  - Use new CompactHeader forwarding APIs in generational mode
>>  - Merge remote-tracking branch 'jdk/master' into merge-latest
>>  - Merge
>>  - 8343649: Shenandoah: ShenandoahEvacInfo event does not follow JFR guidelines
>>    
>>    Reviewed-by: wkemper
>>  - Merge
>>  - 8343227: GenShen: Fold resource mark into management of preselected regions
>>    
>>    Reviewed-by: kdnilsen
>>  - Merge openjdk/jdk tip into great-genshen-pr-redux
>>  - Merge remote-tracking branch 'jdk/master' into merge-latest
>>  - Merge remote-tracking branch 'jdk/master' into merge-latest
>>  - ... and 500 more: https://git.openjdk.org/jdk/compare/889f9062...5e02b5d8
>
> src/hotspot/share/gc/shenandoah/shenandoahCollectionSetPreselector.hpp line 34:
> 
>> 32:   ShenandoahCollectionSet* _cset;
>> 33:   bool* _pset;
>> 34:   ResourceMark _rm;
> 
> Not necessarily for this PR, but something to rectify going forward. It is generally not safe to hide `ResourceMark`-s like this in the objects. Sometimes the callers return stuff allocated in resource area, and if the object like this leaves the scope, it would corrupt the memory. Normally, when we develop resource-area-returning paths, we look around near `ResourceMark`-s to see is any thing fishy is going on. It does not help when `RM` is hidden in the object like this.

Understood. This change was requested earlier in the PR.

> src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.hpp line 76:
> 
>> 74:   void vmop_entry_final_roots();
>> 75: 
>> 76: private:
> 
> Feel free to just make all these `protected`. It is more fuzz to try to provide the minimal possible visibility, especially if it gives the ragged visibility blocks in source. The real reason these are `private` is to protect them from accidental external use. `protected` is also good for this.

https://bugs.openjdk.org/browse/JDK-8344592

> src/hotspot/share/gc/shenandoah/shenandoahHeapRegionCounters.cpp line 49:
> 
>> 47:     const char* cns = PerfDataManager::name_space("shenandoah", "regions");
>> 48:     _name_space = NEW_C_HEAP_ARRAY(char, strlen(cns)+1, mtGC);
>> 49:     strcpy(_name_space, cns); // copy cns into _name_space
> 
> Suggestion:
> 
>     strcpy(_name_space, cns);
> 
> 
> (The comment is self-obvious)

https://bugs.openjdk.org/browse/JDK-8344592

> src/hotspot/share/gc/shenandoah/shenandoahHeapRegionCounters.cpp line 57:
> 
>> 55:     PerfDataManager::create_constant(SUN_GC, cname, PerfData::U_None, num_regions, CHECK);
>> 56: 
>> 57:     cname = PerfDataManager::counter_name(_name_space, "protocol_version"); //creating new protocol_version
> 
> Suggestion:
> 
>     cname = PerfDataManager::counter_name(_name_space, "protocol_version");

https://bugs.openjdk.org/browse/JDK-8344592

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1849309543
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1849308822
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1849309104
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1849309202