On Thu, 2 May 2024 17:23:21 GMT, Aleksey Shipilev <sh...@openjdk.org> 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
