On Mon, 18 Nov 2024 16:20:09 GMT, Aleksey Shipilev <sh...@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
