On Tue, 8 Oct 2024 17:20:31 GMT, William Kemper <wkem...@openjdk.org> 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 478 commits:
> 
>  - Fix merge error
>  - Merge remote-tracking branch 'jdk/master' into great-genshen-pr-redux
>  - Merge remote-tracking branch 'jdk/master' into great-genshen-pr-redux
>  - Merge branch 'shenandoah/master' into great-genshen-pr-redux
>  - Merge
>  - 8341099: GenShen: assert(HAS_FWD == _heap->has_forwarded_objects()) 
> failed: Forwarded object status is sane
>    
>    Reviewed-by: kdnilsen
>  - 8341485: GenShen: Make evac tracker a non-product feature and confine it 
> to generational mode
>    
>    Reviewed-by: kdnilsen, ysr
>  - Merge
>  - 8341042: GenShen: Reset mark bitmaps for unaffiliated regions when 
> preparing for a cycle
>    
>    Reviewed-by: kdnilsen
>  - 8339616: GenShen: Introduce new state to distinguish promote-in-place 
> phase as distinct from concurrent evacuation
>    
>    Reviewed-by: kdnilsen, shade, ysr
>  - ... and 468 more: https://git.openjdk.org/jdk/compare/b9db74a6...4db1e0e1

I looked at the remaining changes, it's mostly tests. Only some minor 
comments/requests on those.

test/hotspot/jtreg/gc/shenandoah/TestAllocIntArrays.java line 100:

> 98: 
> 99: /*
> 100:  * @test id=generational

You are making a test configuration and call it 'generational' but then one of 
the two run configurations doesn't actually run with generational mode. You 
probably want to separate the two?

test/hotspot/jtreg/gc/shenandoah/TestAllocIntArrays.java line 163:

> 161:         final int min = 0;
> 162:         final int max = 384 * 1024;
> 163:         // Each allocated int array is assumed to consume 16 bytes for 
> alignment and header, plus

With the upcoming 'compact headers' it's going to be only 12 bytes. If that 
matters, then the actual number should perhaps be a constant? Preexisting and 
not relevant for this PR, though.

test/hotspot/jtreg/gc/shenandoah/oom/TestAllocLargeObj.java line 1:

> 1: /*

Why are you removing the test?

test/hotspot/jtreg/gc/shenandoah/oom/TestAllocLargerThanHeap.java line 1:

> 1: /*

And this test?

test/hotspot/jtreg/gc/shenandoah/oom/TestAllocOutOfMemory.java line 1:

> 1: /*

Or is this new test subsuming several old tests?

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

Changes requested by rkennke (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2362848068
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1796980759
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1796983485
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1797098313
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1797098581
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1797099580

Reply via email to