RFR: 8237363: Remove automatic is in heap verification in OopIterateClosure

[Thomas Schatzl]

Hi,

On 22.01.20 14:16, Stefan Karlsson wrote:
> 
> 
> On 2020-01-22 12:02, Thomas Schatzl wrote:
>> Hi,
>>
>> 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.
>>>
>>> https://cr.openjdk.java.net/~stefank/8237363/webrev.01/
>>> https://bugs.openjdk.java.net/browse/JDK-8237363
>>>
>>> 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.
> 

Emphasis on "almost".

>>
>> 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.

But with the change people have to think about making sure to do the 
verification manually.
This does not seem an improvement at all.

> 
>   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.
> 

It works for all GCs (+leak profiler) but ZGC given the webrev. This 
does not suggest that this is GC-specific functionality at all. The 
verification method also seems to only uses an innocuous 
CollectedHeap::is_in() call that seems something very basic to support 
for a GC.

What is it in ZGC that prevents CollectedHeap::is_in() to return the 
expected value?

And the opt-out method does have been designed for unusual cases.

Thanks,
   Thomas