RFR: 8392760: Improve liveness/remembered set verification for G1 [v2]
Kim Barrett
kbarrett at openjdk.org
Wed Feb 22 07:06:36 UTC 2023
On Tue, 21 Feb 2023 11:54:15 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:
>> Hi all,
>>
>> can I have reviews for this refactoring/cleanup of the liveness/remembered set verification code for G1?
>>
>> Here is an overview of the changes
>> - previously liveness and remembered set verification could be done separately. Thismade the code a bit more complicated than necessary; now that remembered set verification always follows liveness verification, the code can assume that and do some minor cleanup. I kept the remembered verification a separate closure (`VerifyRemSetClosure`) because the additional boiler plate code is negligible compared to the other duplication that would have been needed around failure count tracking.
>> - factored out liveness and remembered set verification from `HeapRegion::verify`, and heavily cleaned up the resulting `HeapRegion::verify_liveness_and_remembered_set`. This includes some removal of code duplication wrt to the failure count tracking.
>> - factored out a special `G1VerificationClosure::is_oop_safe` method that adds more checking of the `Klass`; I hesitated to put this into `Universe::is_oop` as this is much more extensive checking (and I wanted implicit error reporting :))
>> - CollectedHeap::is_oop() should use `klass_raw` because otherwise it will immediately assert on oops with garbage Klass values (the new code will just return `false` as expected).
>> - some special `oopDesc::print_name_on()` which does not crash on bad oops, similar to existing related debugging helpers.
>>
>> Thanks,
>> Thomas
>
> Thomas Schatzl has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
> - Refactoring after 8302975
> - initial attempts, nothing done
>
> further cleanup
>
> more cleanup
>
> factor out changes that move the verifylivenessclosure
>
> fix compilation
>
> kbarrett review, indentation
>
> Minor removal of unnecessary code
>
> initial version
>
> more cleanup
>
> More improvements
>
> More removal of stuff
>
> More cleanup
>
> More^2 cleanup
>
> Remove some changes
>
> More cleanups
>
> fix compilation
>
> Cleanup
> - removal
src/hotspot/share/gc/g1/heapRegion.cpp line 517:
> 515: LogStream ls(log.error());
> 516:
> 517: MutexLocker x(G1RareEvent_lock, Mutex::_no_safepoint_check_flag);
[pre-existing] It's not clear to me what this lock is protecting. Similarly for the one in `verify_remset`
(in an unmodified part of it).
src/hotspot/share/gc/g1/heapRegion.cpp line 537:
> 535: }
> 536: log.error("----------");
> 537: return true;
There was a `_num_failures++` here in the old code. I think it's still needed, even with the bool return.
-------------
PR: https://git.openjdk.org/jdk/pull/12660
More information about the hotspot-gc-dev
mailing list