Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 17:26:58 GMT, Stefan Karlsson wrote: >> Ah, hm. Indeed! Separate PR? There is some light cleanup in G1 that can be >> associated with it. This PR would keep with just a mechanical rename. > > Sounds like a good idea. Filed: https://bugs.openjdk.org/browse/JDK-8331719 -- I'll give it out to some of our folks as a starter task. - PR Review Comment: https://git.openjdk.org/jdk/pull/19064#discussion_r1590876454
Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 14:40:35 GMT, Aleksey Shipilev wrote: > `CollectedHeap::is_gc_active()` is confusing, since its name implies _any_ GC > phase is running, while it actually only covers the STW GCs. It would be good > to rename it for clarity. The freed-up name, `is_gc_active` could then be > repurposed to track any (concurrent or STW) GC phase running. That would be > useful to resolve [JDK-8331572](https://bugs.openjdk.org/browse/JDK-8331572). > > Doing this rename separately guarantees we have caught and renamed all > current uses. > > Additional testing: > - [x] Linux AArch64 server fastdebug, `all` Thanks all! - PR Comment: https://git.openjdk.org/jdk/pull/19064#issuecomment-2095775823
Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 14:40:35 GMT, Aleksey Shipilev wrote: > `CollectedHeap::is_gc_active()` is confusing, since its name implies _any_ GC > phase is running, while it actually only covers the STW GCs. It would be good > to rename it for clarity. The freed-up name, `is_gc_active` could then be > repurposed to track any (concurrent or STW) GC phase running. That would be > useful to resolve [JDK-8331572](https://bugs.openjdk.org/browse/JDK-8331572). > > Doing this rename separately guarantees we have caught and renamed all > current uses. > > Additional testing: > - [ ] Linux AArch64 server fastdebug, `all` Looks good. - Marked as reviewed by gli (Committer). PR Review: https://git.openjdk.org/jdk/pull/19064#pullrequestreview-2037637061
Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 14:40:35 GMT, Aleksey Shipilev wrote: > `CollectedHeap::is_gc_active()` is confusing, since its name implies _any_ GC > phase is running, while it actually only covers the STW GCs. It would be good > to rename it for clarity. The freed-up name, `is_gc_active` could then be > repurposed to track any (concurrent or STW) GC phase running. That would be > useful to resolve [JDK-8331572](https://bugs.openjdk.org/browse/JDK-8331572). > > Doing this rename separately guarantees we have caught and renamed all > current uses. > > Additional testing: > - [ ] Linux AArch64 server fastdebug, `all` Marked as reviewed by tschatzl (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19064#pullrequestreview-2037494618
Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 14:40:35 GMT, Aleksey Shipilev wrote: > `CollectedHeap::is_gc_active()` is confusing, since its name implies _any_ GC > phase is running, while it actually only covers the STW GCs. It would be good > to rename it for clarity. The freed-up name, `is_gc_active` could then be > repurposed to track any (concurrent or STW) GC phase running. That would be > useful to resolve [JDK-8331572](https://bugs.openjdk.org/browse/JDK-8331572). > > Doing this rename separately guarantees we have caught and renamed all > current uses. > > Additional testing: > - [ ] Linux AArch64 server fastdebug, `all` LGTM - Marked as reviewed by zgu (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19064#pullrequestreview-2036411170
Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 17:23:21 GMT, Aleksey Shipilev wrote:
>> src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1270:
>>
>>> 1268:
>>> 1269: ParallelScavengeHeap* heap = ParallelScavengeHeap::heap();
>>> 1270: assert(!heap->is_stw_gc_active(), "not reentrant");
>>
>> While reading this I see that all these "not reentrant" asserts seems
>> redundant given that we already do these checks in `IsSTWGCActiveMark`.
>> Brownies points if you get rid of them. ;)
>
> Ah, hm. Indeed! Separate PR? There is some light cleanup in G1 that can be
> associated with it. This PR would keep with just a mechanical rename.
Sounds like a good idea.
>> src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1493:
>>
>>> 1491: PCAddThreadRootsMarkingTaskClosure(uint worker_id) :
>>> _worker_id(worker_id) { }
>>> 1492: void do_thread(Thread* thread) {
>>> 1493: assert(ParallelScavengeHeap::heap()->is_stw_gc_active(), "called
>>> outside gc");
>>
>> Should this be updated to "called outside gc pause" as you did in
>> `G1CollectedHeap::pin_object`? The same comment goes for the other
>> occurrences below.
>
> I deliberately stopped myself from doing this for Parallel GC code, where
> every GC is STW GC :) I can change to "GC pause" if you want.
Ah, I see. I wouldn't mind if it were changed to include "pause", but I'm also
OK with you leaving it as is.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19064#discussion_r1588019866
PR Review Comment: https://git.openjdk.org/jdk/pull/19064#discussion_r1588018382
Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 17:04:44 GMT, Stefan Karlsson wrote: >> `CollectedHeap::is_gc_active()` is confusing, since its name implies _any_ >> GC phase is running, while it actually only covers the STW GCs. It would be >> good to rename it for clarity. The freed-up name, `is_gc_active` could then >> be repurposed to track any (concurrent or STW) GC phase running. That would >> be useful to resolve >> [JDK-8331572](https://bugs.openjdk.org/browse/JDK-8331572). >> >> Doing this rename separately guarantees we have caught and renamed all >> current uses. >> >> Additional testing: >> - [ ] Linux AArch64 server fastdebug, `all` > > src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1270: > >> 1268: >> 1269: ParallelScavengeHeap* heap = ParallelScavengeHeap::heap(); >> 1270: assert(!heap->is_stw_gc_active(), "not reentrant"); > > While reading this I see that all these "not reentrant" asserts seems > redundant given that we already do these checks in `IsSTWGCActiveMark`. > Brownies points if you get rid of them. ;) Ah, hm. Indeed! Separate PR? There is some light cleanup in G1 that can be associated with it. This PR would keep with just a mechanical rename. - PR Review Comment: https://git.openjdk.org/jdk/pull/19064#discussion_r1588015870
Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 14:40:35 GMT, Aleksey Shipilev wrote: > `CollectedHeap::is_gc_active()` is confusing, since its name implies _any_ GC > phase is running, while it actually only covers the STW GCs. It would be good > to rename it for clarity. The freed-up name, `is_gc_active` could then be > repurposed to track any (concurrent or STW) GC phase running. That would be > useful to resolve [JDK-8331572](https://bugs.openjdk.org/browse/JDK-8331572). > > Doing this rename separately guarantees we have caught and renamed all > current uses. > > Additional testing: > - [ ] Linux AArch64 server fastdebug, `all` Marked as reviewed by stefank (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19064#pullrequestreview-2036349526
Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 16:56:11 GMT, Stefan Karlsson wrote:
>> `CollectedHeap::is_gc_active()` is confusing, since its name implies _any_
>> GC phase is running, while it actually only covers the STW GCs. It would be
>> good to rename it for clarity. The freed-up name, `is_gc_active` could then
>> be repurposed to track any (concurrent or STW) GC phase running. That would
>> be useful to resolve
>> [JDK-8331572](https://bugs.openjdk.org/browse/JDK-8331572).
>>
>> Doing this rename separately guarantees we have caught and renamed all
>> current uses.
>>
>> Additional testing:
>> - [ ] Linux AArch64 server fastdebug, `all`
>
> src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1493:
>
>> 1491: PCAddThreadRootsMarkingTaskClosure(uint worker_id) :
>> _worker_id(worker_id) { }
>> 1492: void do_thread(Thread* thread) {
>> 1493: assert(ParallelScavengeHeap::heap()->is_stw_gc_active(), "called
>> outside gc");
>
> Should this be updated to "called outside gc pause" as you did in
> `G1CollectedHeap::pin_object`? The same comment goes for the other
> occurrences below.
I deliberately stopped myself from doing this for Parallel GC code, where every
GC is STW GC :) I can change to "GC pause" if you want.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19064#discussion_r1587999105
Re: RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
On Thu, 2 May 2024 14:40:35 GMT, Aleksey Shipilev wrote:
> `CollectedHeap::is_gc_active()` is confusing, since its name implies _any_ GC
> phase is running, while it actually only covers the STW GCs. It would be good
> to rename it for clarity. The freed-up name, `is_gc_active` could then be
> repurposed to track any (concurrent or STW) GC phase running. That would be
> useful to resolve [JDK-8331572](https://bugs.openjdk.org/browse/JDK-8331572).
>
> Doing this rename separately guarantees we have caught and renamed all
> current uses.
>
> Additional testing:
> - [ ] Linux AArch64 server fastdebug, `all`
Seems like a good change.
src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1270:
> 1268:
> 1269: ParallelScavengeHeap* heap = ParallelScavengeHeap::heap();
> 1270: assert(!heap->is_stw_gc_active(), "not reentrant");
While reading this I see that all these "not reentrant" asserts seems redundant
given that we already do these checks in `IsSTWGCActiveMark`. Brownies points
if you get rid of them. ;)
src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1493:
> 1491: PCAddThreadRootsMarkingTaskClosure(uint worker_id) :
> _worker_id(worker_id) { }
> 1492: void do_thread(Thread* thread) {
> 1493: assert(ParallelScavengeHeap::heap()->is_stw_gc_active(), "called
> outside gc");
Should this be updated to "called outside gc pause" as you did in
`G1CollectedHeap::pin_object`? The same comment goes for the other occurrences
below.
-
Changes requested by stefank (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19064#pullrequestreview-2036315901
PR Review Comment: https://git.openjdk.org/jdk/pull/19064#discussion_r1587988542
PR Review Comment: https://git.openjdk.org/jdk/pull/19064#discussion_r1587974562
RFR: 8331573: Rename CollectedHeap::is_gc_active to be explicitly about STW GCs
`CollectedHeap::is_gc_active()` is confusing, since its name implies _any_ GC phase is running, while it actually only covers the STW GCs. It would be good to rename it for clarity. The freed-up name, `is_gc_active` could then be repurposed to track any (concurrent or STW) GC phase running. That would be useful to resolve [JDK-8331572](https://bugs.openjdk.org/browse/JDK-8331572). Doing this rename separately guarantees we have caught and renamed all current uses. Additional testing: - [ ] Linux AArch64 server fastdebug, `all` - Commit messages: - Fix Changes: https://git.openjdk.org/jdk/pull/19064/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19064=00 Issue: https://bugs.openjdk.org/browse/JDK-8331573 Stats: 64 lines in 27 files changed: 0 ins; 2 del; 62 mod Patch: https://git.openjdk.org/jdk/pull/19064.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19064/head:pull/19064 PR: https://git.openjdk.org/jdk/pull/19064
