Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Mon, 25 Nov 2024 18:22:09 GMT, Aleksey Shipilev wrote: >> I'll make some adjustments here. Note that `OLD_MARKING` also implies (and >> sets) `MARKING`, and - unlike `YOUNG_MARKING` - is not exclusive with >> `EVACUTION` and `UPDATEREFS`. Perhaps an explanatory comment is the best >> option here? > > Thing is, the description for `MARKING` says "marking is in progress" (for > any mode), while `YOUNG_MARKING` says that it is only true for yougn marking. > So for a casual reader, checking for `YOUNG_MARKING` in non-generational code > looks weird. I don't know the best way out of this, really. I think this is > the only place when we check `YOUNG_MARKING` for non-gen mode? If so, we > should probably fix it right here? Something like this maybe: https://bugs.openjdk.org/browse/JDK-8344985? - PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1857241797
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Mon, 25 Nov 2024 17:59:05 GMT, William Kemper wrote: >> We should really either predicate it on generational mode (we already poll >> it a few lines below) and check for `MARKING` specifically, or assert it. > > I'll make some adjustments here. Note that `OLD_MARKING` also implies (and > sets) `MARKING`, and - unlike `YOUNG_MARKING` - is not exclusive with > `EVACUTION` and `UPDATEREFS`. Perhaps an explanatory comment is the best > option here? Thing is, the description for `MARKING` says "marking is in progress" (for any mode), while `YOUNG_MARKING` says that it is only true for yougn marking. So for a casual reader, checking for `YOUNG_MARKING` in non-generational code looks weird. I don't know the best way out of this, really. I think this is the only place when we check `YOUNG_MARKING` for non-gen mode? If so, we should probably fix it right here? - PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1857151960
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Thu, 21 Nov 2024 18:30:31 GMT, Aleksey Shipilev wrote: >> Yes, in single gen mode we set `MARKING` and `YOUNG_MARKING` together. See >> https://github.com/openjdk/shenandoah/blob/master/src/hotspot/share/gc/shenandoah/shenandoahGlobalGeneration.cpp#L81 > > We should really either predicate it on generational mode (we already poll it > a few lines below) and check for `MARKING` specifically, or assert it. I'll make some adjustments here. Note that `OLD_MARKING` also implies (and sets) `MARKING`, and - unlike `YOUNG_MARKING` - is not exclusive with `EVACUTION` and `UPDATEREFS`. Perhaps an explanatory comment is the best option here? - PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1857098111
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Thu, 21 Nov 2024 17:02:58 GMT, Aleksey Shipilev wrote: >> Hmm, I find the version in the PR much more readable. How about we upstream >> these changes separately? > > Yes, let's do that, if you have cycles. https://bugs.openjdk.org/browse/JDK-8344797 - PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852976446
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Tue, 19 Nov 2024 23:17:48 GMT, William Kemper wrote:
>> src/hotspot/share/gc/shenandoah/shenandoahMarkClosures.hpp line 34:
>>
>>> 32: class ShenandoahHeapRegion;
>>> 33:
>>> 34: class ShenandoahFinalMarkUpdateRegionStateClosure : public
>>> ShenandoahHeapRegionClosure {
>>
>> There is the `shenandoahHeapRegionClosures.hpp` for these, no?
>
> It wasn't the intention that `shenandoahHeapRegionClosures.hpp` should become
> home to all of the `*HeapRegionClosures`.
> `ShenandoahFinalMarkUpdateRegionStateClosure` is shared in a couple of
> implementation files, but not the same as the ones using
> `shenandoahHeapRegionClosures.hpp`. I think
> `ShenandoahUpdateCensusZeroCohortClosure` should be moved into the only
> implementation file that uses it.
https://bugs.openjdk.org/browse/JDK-8344779
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852709824
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Wed, 20 Nov 2024 00:13:13 GMT, William Kemper wrote:
>> src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp line 420:
>>
>>> 418: }
>>> 419: int gc_state = _heap->gc_state();
>>> 420: if ((gc_state & ShenandoahHeap::YOUNG_MARKING) != 0) {
>>
>> It is not very clear this works fine with single-gen mode. Does `MARKING`
>> imply `YOUNG_MARKING` in single-gen mode? I think we better make it
>> abundantly clear with `is_generational()` checks.
>
> Yes, in single gen mode we set `MARKING` and `YOUNG_MARKING` together. See
> https://github.com/openjdk/shenandoah/blob/master/src/hotspot/share/gc/shenandoah/shenandoahGlobalGeneration.cpp#L81
We should really either predicate it on generational mode (we already poll it a
few lines below) and check for `MARKING` specifically, or assert it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852659199
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Tue, 19 Nov 2024 19:54:45 GMT, William Kemper wrote:
>> src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 276:
>>
>>> 274: log_info(gc)("GC cancellation took %.3fs", cancel_time);
>>> 275: _cancel_requested_time = 0;
>>> 276: }
>>
>> Do we need this? Is this useful?
>
> We could log this at debug? I believe it is useful because this time
> (currently) is not accounted for in the heuristic's notion of 'cycle time'.
Yeah, `gc=debug` would be okay for this.
>> src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.cpp line 193:
>>
>>> 191: Phase phase = Phase(pi);
>>> 192: if (is_worker_phase(phase)) {
>>> 193: double sum = uninitialized();
>>
>> I think you can avoid changing this method to limit the churn.
>
> Hmm, I find the version in the PR much more readable. How about we upstream
> these changes separately?
Yes, let's do that, if you have cycles.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852533117
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852533548
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Wed, 20 Nov 2024 19:55:35 GMT, Aleksey Shipilev 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/shenandoahConcurrentMark.cpp line 62:
>
>> 60: void work(uint worker_id) {
>> 61: ShenandoahHeap* heap = ShenandoahHeap::heap();
>> 62: ShenandoahParallelWorkerSession worker_session(worker_id);
>
> Why this needs to be `ShenandoahParallelWorkerSession`, not
> `ShenandoahConcurrentWorkerSession`? This is a concurrent step, is it?
Yes, https://bugs.openjdk.org/browse/JDK-8344670
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1851158343
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Wed, 20 Nov 2024 16:47:20 GMT, Aleksey Shipilev wrote: >> No, in single gen mode regions that are not `FREE` are affiliated with >> `YOUNG`. > > That's weird. We have `Young`, `Old` and `Global` generations. Why > affiliation is not `Free`, `Young`, `Old`, and `Global` then? Yes, it's a little weird. It didn't seem necessary to have a `Global` affiliation. - PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1850927526
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Tue, 19 Nov 2024 23:07:21 GMT, William Kemper wrote:
>> src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.cpp line 44:
>>
>>> 42: for (size_t idx = 0; idx < num_regions; idx++) {
>>> 43: ShenandoahHeapRegion* r = heap->get_region(idx);
>>> 44: if (r->is_affiliated() && heap->is_bitmap_slice_committed(r) &&
>>> !is_bitmap_clear_range(r->bottom(), r->end())) {
>>
>> I don't understand this for single gen mode. In that mode `is_affiliated()
>> == false` always, right? So this check never passes, and `is_bitmap_clear`
>> always returns `true`?
>
> No, in single gen mode regions that are not `FREE` are affiliated with
> `YOUNG`.
That's weird. We have `Young`, `Old` and `Global` generations. Why affiliation
is not `Free`, `Young`, `Old`, and `Global` then?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1850665665
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Wed, 13 Nov 2024 19:32:53 GMT, William Kemper wrote:
>> This PR merges JEP 404, a generational mode for the Shenandoah garbage
>> collector. The JEP can be viewed here: https://openjdk.org/jeps/404. We
>> would like to target JDK24 with this PR.
>
> 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
Almost there, 220/230 reviewed.
src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp line 62:
> 60: void work(uint worker_id) {
> 61: ShenandoahHeap* heap = ShenandoahHeap::heap();
> 62: ShenandoahParallelWorkerSession worker_session(worker_id);
Why this needs to be `ShenandoahParallelWorkerSession`, not
`ShenandoahConcurrentWorkerSession`? This is a concurrent step, is it?
-
PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2449189030
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1850899803
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Tue, 19 Nov 2024 23:56:56 GMT, William Kemper wrote:
>> src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.cpp line 455:
>>
>>> 453: // Note: would be sufficient to mark only the card that holds the
>>> start of this Reference object.
>>> 454:
>>> heap->old_generation()->card_scan()->mark_range_as_dirty(cast_from_oop(reference),
>>> reference->size());
>>> 455: }
>>
>> Two things:
>> a) This sounds like `card_table_barrier(reference, raw_referent)`? Since
>> this code is getting called for every dropped reference, just checking a
>> `ShenandoahCardBarrier` flag sounds more efficient.
>> b) Is there a point in dirtying up to `reference->size()`?
>
> Will test this.
https://bugs.openjdk.org/browse/JDK-8344640
>> src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 657:
>>
>>> 655: _generation(nullptr) {
>>> 656: if (_options._verify_marked ==
>>> ShenandoahVerifier::_verify_marked_complete_satb_empty) {
>>> 657: Threads::change_thread_claim_token();
>>
>> It is fairly odd to see Verifier touching the claim token, since the bug
>> _may be_ somewhere in parallel thread oop iteration infra. I think it is
>> fine to just use `Threads::threads_do` (non-parallel version), which AFAIU
>> does not require token modifications.
>
> Will test this.
https://bugs.openjdk.org/browse/JDK-8344638
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1850900559
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1850901137
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Wed, 13 Nov 2024 19:32:53 GMT, William Kemper wrote:
>> This PR merges JEP 404, a generational mode for the Shenandoah garbage
>> collector. The JEP can be viewed here: https://openjdk.org/jeps/404. We
>> would like to target JDK24 with this PR.
>
> 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
Some more stuff. 180/230 files done %)
src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp line 420:
> 418: }
> 419: int gc_state = _heap->gc_state();
> 420: if ((gc_state & ShenandoahHeap::YOUNG_MARKING) != 0) {
It is not very clear this works fine with single-gen mode. Does `MARKING` imply
`YOUNG_MARKING` in single-gen mode? I think we better make it abundantly clear
with `is_generational()` checks.
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.
src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 276:
> 274: log_info(gc)("GC cancellation took %.3fs", cancel_time);
> 275: _cancel_requested_time = 0;
> 276: }
Do we need this? Is this useful?
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)
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");
src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.cpp line 193:
> 191: Phase phase = Phase(pi);
> 192: if (is_worker_phase(phase)) {
> 193: double sum = uninitialized();
I think you can avoid changing this method to limit the churn.
src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.cpp line 455:
> 453: // Note: would be sufficient to mark only the card that holds the
> start of this Reference object.
> 454:
> heap->old_generation()->card_scan()->mark_range_as_dirty(cast_from_oop(reference),
> reference->size());
> 455: }
Two things:
a) This sounds like `card_table_barrier(reference, raw_referent)`? Since this
code is getting called for every dropped reference, just checking a
`ShenandoahCardBarrier` flag sounds more efficient.
b) Is there a point in dirtying up to `reference->size()`?
src/hotspot/share/gc/shenandoah/shenandoahSharedVariables.hpp line 125:
> 123: ShenandoahSharedValue ov = Atomic::load_acquire(&value);
> 124: // We require all bits of mask_val to be set
> 125: if ((ov & mask_val) == mask_val) {
This looks like a pre-existing bug, right? I assume this does not affect
current Shenandoah, since we do not set multiple bits. I am fine with doing
this in this PR, but it would probably be good to backport this fix in earlier
releases too.
-
PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2439178281
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1848286637
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1846888999
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844183614
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844187355
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#di
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Tue, 19 Nov 2024 16:46:36 GMT, Aleksey Shipilev 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/shenandoahVerifier.cpp line 657:
>
>> 655: _generation(nullptr) {
>> 656: if (_options._verify_marked ==
>> ShenandoahVerifier::_verify_marked_complete_satb_empty) {
>> 657: Threads::change_thread_claim_token();
>
> It is fairly odd to see Verifier touching the claim token, since the bug _may
> be_ somewhere in parallel thread oop iteration infra. I think it is fine to
> just use `Threads::threads_do` (non-parallel version), which AFAIU does not
> require token modifications.
Will test this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1849329578
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Tue, 19 Nov 2024 12:38:59 GMT, Aleksey Shipilev 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/shenandoahBarrierSet.inline.hpp line 420:
>
>> 418: }
>> 419: int gc_state = _heap->gc_state();
>> 420: if ((gc_state & ShenandoahHeap::YOUNG_MARKING) != 0) {
>
> It is not very clear this works fine with single-gen mode. Does `MARKING`
> imply `YOUNG_MARKING` in single-gen mode? I think we better make it
> abundantly clear with `is_generational()` checks.
Yes, in single gen mode we set `MARKING` and `YOUNG_MARKING` together. See
https://github.com/openjdk/shenandoah/blob/master/src/hotspot/share/gc/shenandoah/shenandoahGlobalGeneration.cpp#L81
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1849325345
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Mon, 18 Nov 2024 16:42:50 GMT, Aleksey Shipilev 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/shenandoahReferenceProcessor.cpp line 455: > >> 453: // Note: would be sufficient to mark only the card that holds the >> start of this Reference object. >> 454: >> heap->old_generation()->card_scan()->mark_range_as_dirty(cast_from_oop(reference), >> reference->size()); >> 455: } > > Two things: > a) This sounds like `card_table_barrier(reference, raw_referent)`? Since > this code is getting called for every dropped reference, just checking a > `ShenandoahCardBarrier` flag sounds more efficient. > b) Is there a point in dirtying up to `reference->size()`? Will test this. - PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1849315332
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Mon, 18 Nov 2024 16:20:09 GMT, Aleksey Shipilev 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
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Tue, 19 Nov 2024 17:35:14 GMT, Aleksey Shipilev 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/shenandoahMarkClosures.hpp line 34:
>
>> 32: class ShenandoahHeapRegion;
>> 33:
>> 34: class ShenandoahFinalMarkUpdateRegionStateClosure : public
>> ShenandoahHeapRegionClosure {
>
> There is the `shenandoahHeapRegionClosures.hpp` for these, no?
It wasn't the intention that `shenandoahHeapRegionClosures.hpp` should become
home to all of the `*HeapRegionClosures`.
`ShenandoahFinalMarkUpdateRegionStateClosure` is shared in a couple of
implementation files, but not the same as the ones using
`shenandoahHeapRegionClosures.hpp`. I think
`ShenandoahUpdateCensusZeroCohortClosure` should be moved into the only
implementation file that uses it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1849213506
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Tue, 19 Nov 2024 17:31:15 GMT, Aleksey Shipilev 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/shenandoahMarkingContext.cpp line 44:
>
>> 42: for (size_t idx = 0; idx < num_regions; idx++) {
>> 43: ShenandoahHeapRegion* r = heap->get_region(idx);
>> 44: if (r->is_affiliated() && heap->is_bitmap_slice_committed(r) &&
>> !is_bitmap_clear_range(r->bottom(), r->end())) {
>
> I don't understand this for single gen mode. In that mode `is_affiliated() ==
> false` always, right? So this check never passes, and `is_bitmap_clear`
> always returns `true`?
No, in single gen mode regions that are not `FREE` are affiliated with `YOUNG`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1849165775
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Mon, 18 Nov 2024 16:22:31 GMT, Aleksey Shipilev 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/shenandoahPhaseTimings.cpp line 193:
>
>> 191: Phase phase = Phase(pi);
>> 192: if (is_worker_phase(phase)) {
>> 193: double sum = uninitialized();
>
> I think you can avoid changing this method to limit the churn.
Hmm, I find the version in the PR much more readable. How about we upstream
these changes separately?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1849160336
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Wed, 13 Nov 2024 19:32:53 GMT, William Kemper wrote:
>> This PR merges JEP 404, a generational mode for the Shenandoah garbage
>> collector. The JEP can be viewed here: https://openjdk.org/jeps/404. We
>> would like to target JDK24 with this PR.
>
> 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
Dropping the rest of my comments from today's read before I sign off. I have
reviewed 200/230 files, will continue tomorrow.
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.
src/hotspot/share/gc/shenandoah/shenandoahMarkClosures.hpp line 34:
> 32: class ShenandoahHeapRegion;
> 33:
> 34: class ShenandoahFinalMarkUpdateRegionStateClosure : public
> ShenandoahHeapRegionClosure {
There is the `shenandoahHeapRegionClosures.hpp` for these, no?
src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.cpp line 44:
> 42: for (size_t idx = 0; idx < num_regions; idx++) {
> 43: ShenandoahHeapRegion* r = heap->get_region(idx);
> 44: if (r->is_affiliated() && heap->is_bitmap_slice_committed(r) &&
> !is_bitmap_clear_range(r->bottom(), r->end())) {
I don't understand this for single gen mode. In that mode `is_affiliated() ==
false` always, right? So this check never passes, and `is_bitmap_clear` always
returns `true`?
src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 657:
> 655: _generation(nullptr) {
> 656: if (_options._verify_marked ==
> ShenandoahVerifier::_verify_marked_complete_satb_empty) {
> 657: Threads::change_thread_claim_token();
It is fairly odd to see Verifier touching the claim token, since the bug _may
be_ somewhere in parallel thread oop iteration infra. I think it is fine to
just use `Threads::threads_do` (non-parallel version), which AFAIU does not
require token modifications.
-
PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2445988182
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1848675048
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1848786419
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1848781508
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1848711178
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Fri, 15 Nov 2024 17:01:29 GMT, Aleksey Shipilev 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/shenandoahHeap.inline.hpp line 276:
>
>> 274: log_info(gc)("GC cancellation took %.3fs", cancel_time);
>> 275: _cancel_requested_time = 0;
>> 276: }
>
> Do we need this? Is this useful?
We could log this at debug? I believe it is useful because this time
(currently) is not accounted for in the heuristic's notion of 'cycle time'.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1848979381
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Fri, 15 Nov 2024 15:12:30 GMT, Martin Doerr wrote: >> The new test TestAllocOutOfMemory.java#large failed on some PPC64 machines: >> >> Exception: java.lang.OutOfMemoryError thrown from the >> UncaughtExceptionHandler in thread "main" >> >> java.lang.RuntimeException: 'java.lang.OutOfMemoryError: Java heap space' >> missing from stdout/stderr >> >> Probably not a show-stopper. Should the test be adapted or disabled for this >> platform? > >> @TheRealMDoerr - do you have the logs from the test failure? If so, could >> you open a ticket with them in JBS? Thank you! > > Filed [JDK-8344312](https://bugs.openjdk.org/browse/JDK-8344312). > > In addition, we see the following tests failing with > "java.lang.OutOfMemoryError: Java heap space" on all Shenandoah platforms: > gc/stress/jfr/TestStressBigAllocationGCEventsWithShenandoah.java#default > gc/stress/jfr/TestStressBigAllocationGCEventsWithShenandoah.java#generational > Can you reproduce those? @TheRealMDoerr , it looks like @ysramakrishna has [filed a ticket already](https://bugs.openjdk.org/browse/JDK-8342786) for the `TestStressBigAllocationGCEventsWithShenandoah` failures. - PR Comment: https://git.openjdk.org/jdk/pull/21273#issuecomment-2484266111
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Fri, 15 Nov 2024 15:12:30 GMT, Martin Doerr wrote: >> The new test TestAllocOutOfMemory.java#large failed on some PPC64 machines: >> >> Exception: java.lang.OutOfMemoryError thrown from the >> UncaughtExceptionHandler in thread "main" >> >> java.lang.RuntimeException: 'java.lang.OutOfMemoryError: Java heap space' >> missing from stdout/stderr >> >> Probably not a show-stopper. Should the test be adapted or disabled for this >> platform? > >> @TheRealMDoerr - do you have the logs from the test failure? If so, could >> you open a ticket with them in JBS? Thank you! > > Filed [JDK-8344312](https://bugs.openjdk.org/browse/JDK-8344312). > > In addition, we see the following tests failing with > "java.lang.OutOfMemoryError: Java heap space" on all Shenandoah platforms: > gc/stress/jfr/TestStressBigAllocationGCEventsWithShenandoah.java#default > gc/stress/jfr/TestStressBigAllocationGCEventsWithShenandoah.java#generational > Can you reproduce those? @TheRealMDoerr - Yes, those tests failed for me. I'll look into them. I'm also working to get my hands on a PPC64 machine to investigate [JDK-8344312](https://bugs.openjdk.org/browse/JDK-8344312). We appreciate your help here! - PR Comment: https://git.openjdk.org/jdk/pull/21273#issuecomment-2480152786
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Fri, 15 Nov 2024 14:11:10 GMT, Aleksey Shipilev 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/shenandoahEvacTracker.cpp line 39:
>
>> 37: if (_use_age_table) {
>> 38: _age_table = new AgeTable(false);
>> 39: }
>
> Sonar caught it: Initialize `_age_table` to `nullptr` for extra safety.
> Current uses seem to be gated by `_use_age_table`, though.
https://bugs.openjdk.org/browse/JDK-8344321
> src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 1856:
>
>> 1854: size_t consumed_old_collector = 0;
>> 1855: size_t consumed_mutator = 0;
>> 1856: size_t available_old = 0;
>
> Sonar: `available_old` is not used. `available_young` is not used.
https://bugs.openjdk.org/browse/JDK-8344321
> src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 380:
>
>> 378:
>> 379: size_t old_evacuated =
>> collection_set->get_old_bytes_reserved_for_evacuation();
>> 380: size_t old_evacuated_committed = (size_t) (ShenandoahOldEvacWaste *
>> old_evacuated);
>
> Sonar: implicit conversion from 'size_t' (aka 'unsigned long') to 'double'
> may lose precision
>
> Not sure if it breaks any math.
https://bugs.openjdk.org/browse/JDK-8344321
> src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp line 656:
>
>> 654: // may not evacuate the entirety of unprocessed candidates in a
>> single mixed evacuation.
>> 655: const size_t max_evac_need = (size_t)
>> 656:
>> (old_generation()->unprocessed_collection_candidates_live_memory() *
>> ShenandoahOldEvacWaste);
>
> Sonar: implicit conversion from 'size_t' (aka 'unsigned long') to 'double'
> may lose precision
>
> Since this is a heuristics calculation, should we be precise here?
https://bugs.openjdk.org/browse/JDK-8344321
> src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp line 89:
>
>> 87: }
>> 88:
>> 89: HeapWord* ShenandoahHeapRegion::allocate(size_t size,
>> ShenandoahAllocRequest req) {
>
> Sonar caught it: `ShenandoahAllocRequest` should be passed by reference here?
https://bugs.openjdk.org/browse/JDK-8344321
> src/hotspot/share/gc/shenandoah/shenandoahHeapRegionCounters.hpp line 99:
>
>> 97: size_t region_size, size_t protocolVersion);
>> 98:
>> 99: uint _count = 0;
>
> Sonar caught it: this `_count` field does not seem to be used.
https://bugs.openjdk.org/browse/JDK-8344321
> src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 157:
>
>> 155: size_t card_at_start = _rs->card_index_for_addr(address);
>> 156: HeapWord* card_start_address =
>> _rs->addr_for_card_index(card_at_start);
>> 157: uint8_t offset_in_card = address - card_start_address;
>
> Sonar: implicit conversion loses integer precision: 'long' to 'uint8_t' (aka
> 'unsigned char').
>
> This looks risky. Should probably be
> `checked_cast(pointer_delta(address, card_start_address, 1))`?
https://bugs.openjdk.org/browse/JDK-8344321
> src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 889:
>
>> 887: size_t previous_group_span = _group_entries[0] * _group_chunk_size[0];
>> 888: for (size_t i = 1; i < _num_groups; i++) {
>> 889: size_t previous_group_entries = (i == 1)? _group_entries[0]:
>> (_group_entries[i-1] - _group_entries[i-2]);
>
> Sonar: Value stored to `previous_group_entries` during its initialization is
> never read
https://bugs.openjdk.org/browse/JDK-8344321
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844218002
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844219136
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844219821
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844219644
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844218123
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844218269
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844219351
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844219014
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Fri, 15 Nov 2024 14:05:51 GMT, Aleksey Shipilev 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/shenandoahGenerationalHeap.cpp line 276:
>
>> 274: // where abundance is defined as >=
>> ShenGenHeap::plab_min_size(). In the former case, we try shrinking the
>> 275: // desired PLAB size to the minimum and retry PLAB
>> allocation to avoid cascading of shared memory allocations.
>> 276: if (plab->words_remaining() < plab_min_size()) {
>
> Sonar caught it: There is a `plab != nullptr` check above at L267, which
> implies `plab` can be `nullptr` here? This would SEGV.
https://bugs.openjdk.org/browse/JDK-8344320
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1844209761
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Thu, 14 Nov 2024 23:39:08 GMT, Martin Doerr 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 > > The new test TestAllocOutOfMemory.java#large failed on some PPC64 machines: > > Exception: java.lang.OutOfMemoryError thrown from the > UncaughtExceptionHandler in thread "main" > > java.lang.RuntimeException: 'java.lang.OutOfMemoryError: Java heap space' > missing from stdout/stderr > > Probably not a show-stopper. Should the test be adapted or disabled for this > platform? > @TheRealMDoerr - do you have the logs from the test failure? If so, could you > open a ticket with them in JBS? Thank you! Filed [JDK-8344312](https://bugs.openjdk.org/browse/JDK-8344312). In addition, we see the following tests failing with "java.lang.OutOfMemoryError: Java heap space" on all Shenandoah platforms: gc/stress/jfr/TestStressBigAllocationGCEventsWithShenandoah.java#default gc/stress/jfr/TestStressBigAllocationGCEventsWithShenandoah.java#generational Can you reproduce those? - PR Comment: https://git.openjdk.org/jdk/pull/21273#issuecomment-2479110413
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Wed, 13 Nov 2024 19:32:53 GMT, William Kemper wrote: >> This PR merges JEP 404, a generational mode for the Shenandoah garbage >> collector. The JEP can be viewed here: https://openjdk.org/jeps/404. We >> would like to target JDK24 with this PR. > > 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/shenandoahScanRemembered.hpp line 361: > 359: // frequently than last byte. This is true when number of clean cards > is greater than number of dirty cards. > 360: static const uint16_t ObjectStartsInCardRegion = 0x80; > 361: static const uint16_t FirstStartBits = 0x7f; I see these are used to do operations against `uint8_t` (`offsets.first`). Any reason why these should not be `uint8_t`? - PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843980993
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Wed, 13 Nov 2024 19:32:53 GMT, William Kemper wrote:
>> This PR merges JEP 404, a generational mode for the Shenandoah garbage
>> collector. The JEP can be viewed here: https://openjdk.org/jeps/404. We
>> would like to target JDK24 with this PR.
>
> 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
Sonar findings:
src/hotspot/share/gc/shenandoah/shenandoahEvacTracker.cpp line 39:
> 37: if (_use_age_table) {
> 38: _age_table = new AgeTable(false);
> 39: }
Sonar caught it: Initialize `_age_table` to `nullptr` for extra safety. Current
uses seem to be gated by `_use_age_table`, though.
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 1505:
> 1503: size_t transferred_regions = 0;
> 1504: ShenandoahLeftRightIterator iterator(&_partitions, which_collector,
> true);
> 1505: idx_t rightmost = _partitions.rightmost_empty(which_collector);
Sonar: `rightmost` is not used.
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 1567:
> 1565: <=
> _partitions.rightmost(ShenandoahFreeSetPartitionId::Collector))) {
> 1566: ShenandoahHeapLocker locker(_heap->lock());
> 1567: max_xfer_regions -=
Sonar: Value stored to `max_xfer_regions` here is not used.
src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 1856:
> 1854: size_t consumed_old_collector = 0;
> 1855: size_t consumed_mutator = 0;
> 1856: size_t available_old = 0;
Sonar: `available_old` is not used. `available_young` is not used.
src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 380:
> 378:
> 379: size_t old_evacuated =
> collection_set->get_old_bytes_reserved_for_evacuation();
> 380: size_t old_evacuated_committed = (size_t) (ShenandoahOldEvacWaste *
> old_evacuated);
Sonar: implicit conversion from 'size_t' (aka 'unsigned long') to 'double' may
lose precision
Not sure if it breaks any math.
src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 455:
> 453: // excess_old < unaffiliated old: we can give back
> MIN(excess_old/region_size_bytes, unaffiliated_old_regions)
> 454: size_t excess_regions = excess_old / region_size_bytes;
> 455: size_t regions_to_xfer = MIN2(excess_regions,
> unaffiliated_old_regions);
Sonar: Value stored to 'regions_to_xfer' during its initialization is never read
src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 549:
> 547: }
> 548: if (r->age() >= tenuring_threshold) {
> 549: if ((r->garbage() < old_garbage_threshold)) {
Sonar: double parentheses.
src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp line 276:
> 274: // where abundance is defined as >=
> ShenGenHeap::plab_min_size(). In the former case, we try shrinking the
> 275: // desired PLAB size to the minimum and retry PLAB
> allocation to avoid cascading of shared memory allocations.
> 276: if (plab->words_remaining() < plab_min_size()) {
Sonar caught it: There is a `plab != nullptr` check above at L267, which
implies `plab` can be `nullptr` here? This would SEGV.
src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp line 656:
> 654: // may not evacuate the entirety of unprocessed candidates in a
> single mixed evacuation.
> 655: const size_t max_evac_need = (size_t)
> 656:
> (old_generation()->unprocessed_collection_candidates_live_memory() *
> ShenandoahOldEvacWaste);
Sonar: implicit conversion from 'size_t' (aka 'unsigned long') to 'double' may
lose precision
Since this is a heuristics calculation, should we be precise here?
src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp line 1003:
> 1001: }
> 1002:
> 1003: namespace ShenandoahCompositeRegionClosure {
We usually avoid `namespace`-s. See Hotspot style guide:
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md#namespaces
src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp line 89:
> 87: }
> 88:
> 89: HeapWord* ShenandoahHeapRegion::allocate(size_t size,
> ShenandoahAllocRequest req) {
Sonar caught it: `ShenandoahAllocRequest` should be passed by reference here?
src/hotspot/share/gc/shenandoah/shenandoahHeapRegionCounter
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Thu, 14 Nov 2024 19:26:12 GMT, Aleksey Shipilev 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/shenandoahGenerationSizer.cpp line 141:
>
>> 139: dst->increase_capacity(bytes_to_transfer);
>> 140: const size_t new_size = dst->max_capacity();
>> 141: log_info(gc)("Transfer " SIZE_FORMAT " region(s) from %s to %s,
>> yielding increased size: " PROPERFMT,
>
> This should not be `log_info(gc)`. In fact, please look at other places where
> you print logging. Generally, `log_info(gc)` should provide _only_ the
> high-level GC info: which phases were running. Everything else should go
> under `log_debug(gc)`.
https://bugs.openjdk.org/browse/JDK-8344263
> src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp line 140:
>
>> 138: break;
>> 139: }
>> 140: case OLD:
>
> Are we not doing OLD STW mark? Deserves a comment.
https://bugs.openjdk.org/browse/JDK-8344264
> src/hotspot/share/gc/shenandoah/shenandoahYoungGeneration.hpp line 54:
>
>> 52: void parallel_heap_region_iterate(ShenandoahHeapRegionClosure* cl)
>> override;
>> 53:
>> 54: void parallel_region_iterate_free(ShenandoahHeapRegionClosure* cl)
>> override;
>
> Is this a sibling of `parallel_heap_region_iterate`? Shouldn't these be
> called `parallel_heap_region_iterate_free`?
https://bugs.openjdk.org/browse/JDK-8344264
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843073877
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843075157
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1843075212
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Thu, 14 Nov 2024 23:39:08 GMT, Martin Doerr 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 > > The new test TestAllocOutOfMemory.java#large failed on some PPC64 machines: > > Exception: java.lang.OutOfMemoryError thrown from the > UncaughtExceptionHandler in thread "main" > > java.lang.RuntimeException: 'java.lang.OutOfMemoryError: Java heap space' > missing from stdout/stderr > > Probably not a show-stopper. Should the test be adapted or disabled for this > platform? @TheRealMDoerr - do you have the logs from the test failure? If so, could you open a ticket with them in JBS? Thank you! - PR Comment: https://git.openjdk.org/jdk/pull/21273#issuecomment-2477671400
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Wed, 13 Nov 2024 19:32:53 GMT, William Kemper wrote: >> This PR merges JEP 404, a generational mode for the Shenandoah garbage >> collector. The JEP can be viewed here: https://openjdk.org/jeps/404. We >> would like to target JDK24 with this PR. > > 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 The new test TestAllocOutOfMemory.java#large failed on some PPC64 machines: Exception: java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in thread "main" java.lang.RuntimeException: 'java.lang.OutOfMemoryError: Java heap space' missing from stdout/stderr Probably not a show-stopper. Should the test be adapted or disabled for this platform? - PR Comment: https://git.openjdk.org/jdk/pull/21273#issuecomment-2477628609
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Wed, 13 Nov 2024 19:32:53 GMT, William Kemper wrote:
>> This PR merges JEP 404, a generational mode for the Shenandoah garbage
>> collector. The JEP can be viewed here: https://openjdk.org/jeps/404. We
>> would like to target JDK24 with this PR.
>
> 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
More reviews...
src/hotspot/share/gc/shenandoah/shenandoahGenerationSizer.cpp line 141:
> 139: dst->increase_capacity(bytes_to_transfer);
> 140: const size_t new_size = dst->max_capacity();
> 141: log_info(gc)("Transfer " SIZE_FORMAT " region(s) from %s to %s,
> yielding increased size: " PROPERFMT,
This should not be `log_info(gc)`. In fact, please look at other places where
you print logging. Generally, `log_info(gc)` should provide _only_ the
high-level GC info: which phases were running. Everything else should go under
`log_debug(gc)`.
src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp line 140:
> 138: break;
> 139: }
> 140: case OLD:
Are we not doing OLD STW mark? Deserves a comment.
src/hotspot/share/gc/shenandoah/shenandoahYoungGeneration.hpp line 54:
> 52: void parallel_heap_region_iterate(ShenandoahHeapRegionClosure* cl)
> override;
> 53:
> 54: void parallel_region_iterate_free(ShenandoahHeapRegionClosure* cl)
> override;
Is this a sibling of `parallel_heap_region_iterate`? Shouldn't these be called
`parallel_heap_region_iterate_free`?
-
PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2436882261
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1842779639
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1842701030
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1842704295
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
On Tue, 12 Nov 2024 19:41:14 GMT, Aleksey Shipilev 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 > > test/hotspot/jtreg/ProblemList.txt line 98: > >> 96: gc/TestAlwaysPreTouchBehavior.java#Epsilon 8334513 generic-all >> 97: gc/stress/gclocker/TestExcessGCLockerCollections.java 8229120 generic-all >> 98: > > This change is unnecessary. https://bugs.openjdk.org/browse/JDK-8344151 - PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1841321018
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v7]
> This PR merges JEP 404, a generational mode for the Shenandoah garbage > collector. The JEP can be viewed here: https://openjdk.org/jeps/404. We would > like to target JDK24 with this PR. 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 - Changes: https://git.openjdk.org/jdk/pull/21273/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21273&range=06 Stats: 22977 lines in 231 files changed: 21234 ins; 886 del; 857 mod Patch: https://git.openjdk.org/jdk/pull/21273.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21273/head:pull/21273 PR: https://git.openjdk.org/jdk/pull/21273
