RFR: 8237363: Remove automatic is in heap verification in OopIterateClosure
stefan.karlsson at oracle.com
Wed Jan 22 13:16:53 UTC 2020
On 2020-01-22 12:02, Thomas Schatzl wrote:
> On 17.01.20 14:31, Stefan Karlsson wrote:
>> Hi all,
>> Please review this patch to remove the automatic "is in heap"
>> verification from OopIterateClosure.
>> OopIterateClosure provides some automatic verification that loaded
>> objects are inside the heap. Closures can opt out from this by
>> overriding should_verify_oops().
>> I propose that we move this verification, and the way to turn it off,
>> and instead let the implementations of the closures decide the kind of
>> verification that is appropriate. I want to do this to de-clutter the
>> closure APIs a bit.
> While the change is correct, I am not really convinced it is a good idea
> to trade verification in one place to the same verification in many place.
An alternative would be to simply remove the verification altogether. As
I said, we almost always check the result of the object address.
> The closure API does not seem to be particularly "cluttered up" by this
> particular API to me.
It's a slippery slope. Previously, we had a lot of GC specific functions
in these interfaces. I've been cleaning this over the years, and this is
one of the last non-essential parts of that interface that implementors
need to consider.
With my removal people don't have to think about this anymore.
It is true that other code typically has many
> other asserts that would fail anyway, but it would be an additional
> safety net when writing new closures.
It's a safety net that works for G1, but almost always is incorrectly
trips in the assert with ZGC.
> This is not a hard no for this change, but is there something else you
> are planning to do in this area where this code would be in the way?
>> I've gone through all OopIterateClosures that don't override
>> should_verify_oops() and added calls to
>> assert_oop_field_points_to_object_in_heap[_or_null] where the closures
>> didn't have equivalent checks.
>> A lot of the places didn't explicitly check that the object is within
>> the heap but they would check for other things like:
>> - Is the corresponding bit index within the range
>> - Is the heap region index within range
>> - Is the object in the reserved heap range (weaker than is_in)
>> I've added asserts to those places. If you think I should remove some
>> of them, please let me now.
>> Tested with tier1-3
More information about the hotspot-gc-dev