RFR: 8302206: Factor out duplicate G1VerificationClosure [v3]

Albert Mingkun Yang ayang at openjdk.org
Mon Feb 13 13:56:30 UTC 2023


On Mon, 13 Feb 2023 11:38:56 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hi all,
>> 
>>   can I have reviews for this cleanup that merges `G1VerifyLiveClosure` from `g1FullGCMarker.cpp` and `VerifyLiveClosure` from `heapRegion.cpp` into a single one as they are virtually identical.
>> 
>> There is probably one change that needs some discussion, and that is about changing the type and meaning of `G1MaxVerifyFailures`: after changing the type of `G1VerificationClosure::_num_failures` to uint, the given comparison does not work any more due to differing types.
>> However the change of the default value to `0` meaning "show all messages" does not seem too bad: even before this change, a value of `0` at least showed one failure as this check (now it shows all messages) is located after the error printing in the closures.
>> It's a develop flag too, so instead of casting around I chose to simply change the meaning.
>> 
>> Testing: gha
>> 
>> Thanks,
>>   Thomas
>
> Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improve G1MaxVerifyFailures bounds

Marked as reviewed by ayang (Reviewer).

src/hotspot/share/gc/g1/heapRegion.cpp line 477:

> 475:     oop obj = CompressedOops::decode_raw_not_null(heap_oop);
> 476: 
> 477:     HeapRegion* from = _g1h->heap_region_containing_or_null(p);

Why `or_null`? I'd expect there's a valid region for `p`, since the containing obj is live.

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

PR: https://git.openjdk.org/jdk/pull/12511


More information about the hotspot-gc-dev mailing list