Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v8]
On Thu, 21 Nov 2024 13:22:42 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 524 commits: >> >> - Merge remote-tracking branch 'shenandoah/master' into >> great-genshen-pr-redux >> - 8344670: GenShen: Use concurrent worker session for concurrent mark phase >> >>Reviewed-by: kdnilsen >> - 8344640: GenShen: Reuse existing card mark barrier function when dropping >> references >> >>Reviewed-by: shade, ysr >> - 8344592: GenShen: Remove unnecessary comments and changes >> >>Reviewed-by: kdnilsen >> - 8344263: GenShen: Reduce extraneous log messages at INFO level >> >>Reviewed-by: ysr, shade >> - 8344638: GenShen: Verifier should not touch claim token >> >>Reviewed-by: shade >> - Merge >> - 8344320: GenShen: Possible null pointer usage in shGenerationalHeap >> >>Reviewed-by: wkemper, ysr >> - 8344321: GenShen: Fix various sonar scan warnings >> >>Reviewed-by: kdnilsen, shade >> - 8344264: GenShen: Improve comments and method names >> >>Reviewed-by: shade >> - ... and 514 more: https://git.openjdk.org/jdk/compare/95a00f8a...7ab16403 > > src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 881: > >> 879: // update costs on slow path. >> 880: monitoring_support()->notify_heap_changed(); >> 881: _heap_changed.set(); > > Why not leave `try_set` intact? This alleviates contention on the shared > counter, AFAICS. https://bugs.openjdk.org/browse/JDK-8344797 - PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852952949
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v8]
On Thu, 21 Nov 2024 20:20:12 GMT, Y. Srinivas Ramakrishna wrote: >> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1972: >> >>> 1970: // Check that if concurrent weak root is set then active_gen isn't >>> null >>> 1971: assert(!is_concurrent_weak_root_in_progress() || >>> active_generation() != nullptr, "Error"); >>> 1972: shenandoah_assert_generations_reconciled(); >> >> Why all of this is checked here? I would have thought `gc_state` machinery >> should only check things related to gc-state. > > Likely debugging detritus that I unintentionally left behind. Feel free to > delete. > I think the `shenandoah_assert_generations_reconciled()` is already checked > in a most spots where it's needed. https://bugs.openjdk.org/browse/JDK-8344797 - PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852949534
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v8]
On Thu, 21 Nov 2024 20:10:20 GMT, William Kemper wrote: >> src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.cpp line 64: >> >>> 62: } >>> 63: } >>> 64: return _mark_bit_map.is_bitmap_clear_range(start, end); >> >> Comprehension check: this seems to bail out the moment it discovers an >> uncommitted slice. Does it really happen? More worryingly, if there is a mix >> of committed and uncommitted chunks, this method returns `true`, even if >> committed chunks are not actually clean? > > In all (3) usages of this method, the `start` and `end` are within the same > region, so it isn't really iterating across regions. I'll rewrite it to > handle this case more clearly. https://bugs.openjdk.org/browse/JDK-8344797 - PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852949662
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v8]
On Thu, 21 Nov 2024 13:50:42 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 524 commits: >> >> - Merge remote-tracking branch 'shenandoah/master' into >> great-genshen-pr-redux >> - 8344670: GenShen: Use concurrent worker session for concurrent mark phase >> >>Reviewed-by: kdnilsen >> - 8344640: GenShen: Reuse existing card mark barrier function when dropping >> references >> >>Reviewed-by: shade, ysr >> - 8344592: GenShen: Remove unnecessary comments and changes >> >>Reviewed-by: kdnilsen >> - 8344263: GenShen: Reduce extraneous log messages at INFO level >> >>Reviewed-by: ysr, shade >> - 8344638: GenShen: Verifier should not touch claim token >> >>Reviewed-by: shade >> - Merge >> - 8344320: GenShen: Possible null pointer usage in shGenerationalHeap >> >>Reviewed-by: wkemper, ysr >> - 8344321: GenShen: Fix various sonar scan warnings >> >>Reviewed-by: kdnilsen, shade >> - 8344264: GenShen: Improve comments and method names >> >>Reviewed-by: shade >> - ... and 514 more: https://git.openjdk.org/jdk/compare/95a00f8a...7ab16403 > > src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.cpp line 64: > >> 62: } >> 63: } >> 64: return _mark_bit_map.is_bitmap_clear_range(start, end); > > Comprehension check: this seems to bail out the moment it discovers an > uncommitted slice. Does it really happen? More worryingly, if there is a mix > of committed and uncommitted chunks, this method returns `true`, even if > committed chunks are not actually clean? In all (3) usages of this method, the `start` and `end` are within the same region, so it isn't really iterating across regions. I'll rewrite it to handle this case more clearly. - PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852802615
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v8]
On Thu, 21 Nov 2024 13:34: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 524 commits: >> >> - Merge remote-tracking branch 'shenandoah/master' into >> great-genshen-pr-redux >> - 8344670: GenShen: Use concurrent worker session for concurrent mark phase >> >>Reviewed-by: kdnilsen >> - 8344640: GenShen: Reuse existing card mark barrier function when dropping >> references >> >>Reviewed-by: shade, ysr >> - 8344592: GenShen: Remove unnecessary comments and changes >> >>Reviewed-by: kdnilsen >> - 8344263: GenShen: Reduce extraneous log messages at INFO level >> >>Reviewed-by: ysr, shade >> - 8344638: GenShen: Verifier should not touch claim token >> >>Reviewed-by: shade >> - Merge >> - 8344320: GenShen: Possible null pointer usage in shGenerationalHeap >> >>Reviewed-by: wkemper, ysr >> - 8344321: GenShen: Fix various sonar scan warnings >> >>Reviewed-by: kdnilsen, shade >> - 8344264: GenShen: Improve comments and method names >> >>Reviewed-by: shade >> - ... and 514 more: https://git.openjdk.org/jdk/compare/95a00f8a...7ab16403 > > src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1972: > >> 1970: // Check that if concurrent weak root is set then active_gen isn't >> null >> 1971: assert(!is_concurrent_weak_root_in_progress() || active_generation() >> != nullptr, "Error"); >> 1972: shenandoah_assert_generations_reconciled(); > > Why all of this is checked here? I would have thought `gc_state` machinery > should only check things related to gc-state. Likely debugging detritus that I unintentionally left behind. Feel free to delete. I think the `shenandoah_assert_generations_reconciled()` is already checked in a most spots where it's needed. - PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852820087
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v8]
On Thu, 21 Nov 2024 18:12:43 GMT, William Kemper wrote:
>> src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 514:
>>
>>> 512:
>>> 513: public:
>>> 514: ShenandoahController* control_thread() { return _control_thread; }
>>
>> This method and field is probably `controller` then, right?
>
> Left the accessor and field with the same name to reduce churn. Clients of
> this method don't really need to change because in most (all?) cases, the
> public API of `ShenandoahControlThread` is declared in `ShenandoahController`.
OK.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852731729
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v8]
On Thu, 21 Nov 2024 09:14:57 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 524 commits:
>>
>> - Merge remote-tracking branch 'shenandoah/master' into
>> great-genshen-pr-redux
>> - 8344670: GenShen: Use concurrent worker session for concurrent mark phase
>>
>>Reviewed-by: kdnilsen
>> - 8344640: GenShen: Reuse existing card mark barrier function when dropping
>> references
>>
>>Reviewed-by: shade, ysr
>> - 8344592: GenShen: Remove unnecessary comments and changes
>>
>>Reviewed-by: kdnilsen
>> - 8344263: GenShen: Reduce extraneous log messages at INFO level
>>
>>Reviewed-by: ysr, shade
>> - 8344638: GenShen: Verifier should not touch claim token
>>
>>Reviewed-by: shade
>> - Merge
>> - 8344320: GenShen: Possible null pointer usage in shGenerationalHeap
>>
>>Reviewed-by: wkemper, ysr
>> - 8344321: GenShen: Fix various sonar scan warnings
>>
>>Reviewed-by: kdnilsen, shade
>> - 8344264: GenShen: Improve comments and method names
>>
>>Reviewed-by: shade
>> - ... and 514 more: https://git.openjdk.org/jdk/compare/95a00f8a...7ab16403
>
> src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 134:
>
>> 132: bool is_thread_safe() override { return true; }
>> 133: };
>> 134:
>
> This looks like belonging in one of the `*closures.hpp` header.
https://bugs.openjdk.org/browse/JDK-8344779
> src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 754:
>
>> 752: oop try_evacuate_object(oop src, Thread* thread,
>> ShenandoahHeapRegion* from_region, ShenandoahAffiliation target_gen);
>> 753: public:
>> 754:
>
> I think this new line should be before `public:`
Sneaking this into: https://bugs.openjdk.org/browse/JDK-8344779
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852709552
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852711083
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v8]
On Thu, 21 Nov 2024 10:16:16 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 524 commits:
>>
>> - Merge remote-tracking branch 'shenandoah/master' into
>> great-genshen-pr-redux
>> - 8344670: GenShen: Use concurrent worker session for concurrent mark phase
>>
>>Reviewed-by: kdnilsen
>> - 8344640: GenShen: Reuse existing card mark barrier function when dropping
>> references
>>
>>Reviewed-by: shade, ysr
>> - 8344592: GenShen: Remove unnecessary comments and changes
>>
>>Reviewed-by: kdnilsen
>> - 8344263: GenShen: Reduce extraneous log messages at INFO level
>>
>>Reviewed-by: ysr, shade
>> - 8344638: GenShen: Verifier should not touch claim token
>>
>>Reviewed-by: shade
>> - Merge
>> - 8344320: GenShen: Possible null pointer usage in shGenerationalHeap
>>
>>Reviewed-by: wkemper, ysr
>> - 8344321: GenShen: Fix various sonar scan warnings
>>
>>Reviewed-by: kdnilsen, shade
>> - 8344264: GenShen: Improve comments and method names
>>
>>Reviewed-by: shade
>> - ... and 514 more: https://git.openjdk.org/jdk/compare/95a00f8a...7ab16403
>
> src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 514:
>
>> 512:
>> 513: public:
>> 514: ShenandoahController* control_thread() { return _control_thread; }
>
> This method and field is probably `controller` then, right?
Left the accessor and field with the same name to reduce churn. Clients of this
method don't really need to change because in most (all?) cases, the public API
of `ShenandoahControlThread` is declared in `ShenandoahController`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852636871
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v8]
On Thu, 21 Nov 2024 10:15:27 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 524 commits:
>>
>> - Merge remote-tracking branch 'shenandoah/master' into
>> great-genshen-pr-redux
>> - 8344670: GenShen: Use concurrent worker session for concurrent mark phase
>>
>>Reviewed-by: kdnilsen
>> - 8344640: GenShen: Reuse existing card mark barrier function when dropping
>> references
>>
>>Reviewed-by: shade, ysr
>> - 8344592: GenShen: Remove unnecessary comments and changes
>>
>>Reviewed-by: kdnilsen
>> - 8344263: GenShen: Reduce extraneous log messages at INFO level
>>
>>Reviewed-by: ysr, shade
>> - 8344638: GenShen: Verifier should not touch claim token
>>
>>Reviewed-by: shade
>> - Merge
>> - 8344320: GenShen: Possible null pointer usage in shGenerationalHeap
>>
>>Reviewed-by: wkemper, ysr
>> - 8344321: GenShen: Fix various sonar scan warnings
>>
>>Reviewed-by: kdnilsen, shade
>> - 8344264: GenShen: Improve comments and method names
>>
>>Reviewed-by: shade
>> - ... and 514 more: https://git.openjdk.org/jdk/compare/95a00f8a...7ab16403
>
> src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 663:
>
>> 661: // -- CDS archive support
>> 662:
>> 663: bool can_load_archived_objects() const override { return
>> !ShenandoahCardBarrier; }
>
> This means CDS heap loading is not yet working with generational Shenandoah?
> This looks OK for the experimental code. Please submit a bug for this and
> assign it to me. I will take a look at it some time later.
Yes, we have: https://bugs.openjdk.org/browse/JDK-8339182
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852633232
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v8]
On Thu, 21 Nov 2024 00:59:27 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 524 commits:
>
> - Merge remote-tracking branch 'shenandoah/master' into
> great-genshen-pr-redux
> - 8344670: GenShen: Use concurrent worker session for concurrent mark phase
>
>Reviewed-by: kdnilsen
> - 8344640: GenShen: Reuse existing card mark barrier function when dropping
> references
>
>Reviewed-by: shade, ysr
> - 8344592: GenShen: Remove unnecessary comments and changes
>
>Reviewed-by: kdnilsen
> - 8344263: GenShen: Reduce extraneous log messages at INFO level
>
>Reviewed-by: ysr, shade
> - 8344638: GenShen: Verifier should not touch claim token
>
>Reviewed-by: shade
> - Merge
> - 8344320: GenShen: Possible null pointer usage in shGenerationalHeap
>
>Reviewed-by: wkemper, ysr
> - 8344321: GenShen: Fix various sonar scan warnings
>
>Reviewed-by: kdnilsen, shade
> - 8344264: GenShen: Improve comments and method names
>
>Reviewed-by: shade
> - ... and 514 more: https://git.openjdk.org/jdk/compare/95a00f8a...7ab16403
229/229 files read! This is an impressive, monumental piece of work, kudos.
More comments from this read follow. I am going to circle back to some open
threads in this PR. I am also running local tests, and in my ad-hoc performance
tests generational mode performs admirably well.
src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 881:
> 879: // update costs on slow path.
> 880: monitoring_support()->notify_heap_changed();
> 881: _heap_changed.set();
Why not leave `try_set` intact? This alleviates contention on the shared
counter, AFAICS.
src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1972:
> 1970: // Check that if concurrent weak root is set then active_gen isn't
> null
> 1971: assert(!is_concurrent_weak_root_in_progress() || active_generation()
> != nullptr, "Error");
> 1972: shenandoah_assert_generations_reconciled();
Why all of this is checked here? I would have thought `gc_state` machinery
should only check things related to gc-state.
src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 134:
> 132: bool is_thread_safe() override { return true; }
> 133: };
> 134:
This looks like belonging in one of the `*closures.hpp` header.
src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 514:
> 512:
> 513: public:
> 514: ShenandoahController* control_thread() { return _control_thread; }
This method and field is probably `controller` then, right?
src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 663:
> 661: // -- CDS archive support
> 662:
> 663: bool can_load_archived_objects() const override { return
> !ShenandoahCardBarrier; }
This means CDS heap loading is not yet working with generational Shenandoah?
This looks OK for the experimental code. Please submit a bug for this and
assign it to me. I will take a look at it some time later.
src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 754:
> 752: oop try_evacuate_object(oop src, Thread* thread, ShenandoahHeapRegion*
> from_region, ShenandoahAffiliation target_gen);
> 753: public:
> 754:
I think this new line should be before `public:`
src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.cpp line 64:
> 62: }
> 63: }
> 64: return _mark_bit_map.is_bitmap_clear_range(start, end);
Comprehension check: this seems to bail out the moment it discovers an
uncommitted slice. Does it really happen? More worryingly, if there is a mix of
committed and uncommitted chunks, this method returns `true`, even if committed
chunks are not actually clean?
-
PR Review: https://git.openjdk.org/jdk/pull/21273#pullrequestreview-2450709063
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852054187
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852075850
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1851646208
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1851763351
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1851761345
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1851765188
PR Review Comment: https://git.openjdk.org/jdk/pull/21273#discussion_r1852139996
Re: RFR: 8337511: Implement JEP 404: Generational Shenandoah (Experimental) [v8]
> 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 524 commits: - Merge remote-tracking branch 'shenandoah/master' into great-genshen-pr-redux - 8344670: GenShen: Use concurrent worker session for concurrent mark phase Reviewed-by: kdnilsen - 8344640: GenShen: Reuse existing card mark barrier function when dropping references Reviewed-by: shade, ysr - 8344592: GenShen: Remove unnecessary comments and changes Reviewed-by: kdnilsen - 8344263: GenShen: Reduce extraneous log messages at INFO level Reviewed-by: ysr, shade - 8344638: GenShen: Verifier should not touch claim token Reviewed-by: shade - Merge - 8344320: GenShen: Possible null pointer usage in shGenerationalHeap Reviewed-by: wkemper, ysr - 8344321: GenShen: Fix various sonar scan warnings Reviewed-by: kdnilsen, shade - 8344264: GenShen: Improve comments and method names Reviewed-by: shade - ... and 514 more: https://git.openjdk.org/jdk/compare/95a00f8a...7ab16403 - Changes: https://git.openjdk.org/jdk/pull/21273/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21273&range=07 Stats: 22960 lines in 230 files changed: 21196 ins; 892 del; 872 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
