RFR: 8231113: Adjust CollectedHeap::check_oop_location()

[Stefan Karlsson]

On 2019-09-19 13:23, Per Liden wrote:
> Hi Stefan,
> 
> On 2019-09-17 21:48, Stefan Karlsson wrote:
>> Hi Per,
>>
>> I think both names are bad because the functions return false 
>> positives but the names give the impression that they won't. For example:
>>
>>     // CMS forwards some non-heap value into the mark oop to reserve 
>> oops during
>>     // promotion, so we can't assert about obj alignment or that the 
>> forwardee is in heap
>> - virtual void check_oop_location(void* addr) const {}
>> + virtual bool is_oop_location(void* addr) const { return true; }
>>
>> It's obvious that this function returns true for pointers outside of 
>> the heap. Other implementations are not as bad, but they also return 
>> false positives. Users can't use is_oop_location to drive code logic, 
>> only to do sanity checks.
>>
>> The check_oop_location functions didn't have that problem, but they 
>> couldn't be used in asserts. Did you consider changing the 
>> check_oop_location functions to return a bool instead of asserting?
>>
>>     G1CollectedHeap* g1h = G1CollectedHeap::heap();
>>     // can't do because of races
>>     // assert(oopDesc::is_oop_or_null(obj), "expected an oop");
>> - g1h->check_oop_location(obj);
>> + assert(g1h->check_oop_location(obj), "invalid oop location");
>>
>> I think that would be less misleading to readers of the code.
> 
> I see your point. After looking more at how we use check_oop_location() 
> today, I'm more and more thinking that it's just the wrong abstraction. 
> In CMS we're "checking oops" that we know aren't oops, so we're forced 
> to disable check_oop_location() completely. The checks done for filler 
> objects is sort of strange too. We're filling a space outside of the 
> allocated part of a TLAB/region, so it becomes a philosophical 
> discussion, where this is no "correct" answer, whether that is a valid 
> oop location or not.
> 
> I actually think I'd prefer removing check_oop_location() all together, 
> with the rational that some of the asserts where it's used today are 
> "conceptually wrong". In the other cases (e.g. in CompressedOops and 
> G1), we can make valid checks using better mechanisms.
> 
> So, how about this:
> 
> http://cr.openjdk.java.net/~pliden/8231113/webrev.1

Looks good to me!

StefanK

> 
> Passed tier1-3.
> 
> cheers,
> Per
> 
>>
>> Thanks,
>> StefanK
>>
>> On 2019-09-17 15:02, Per Liden wrote:
>>> I propose we adjust and rename CollectedHeap::check_oop_location() 
>>> into CollectedHeap::is_oop_location() that returns a bool, to better 
>>> match its sibling CollectedHeap::is_oop() and how that is used.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8231113
>>> Webrev: http://cr.openjdk.java.net/~pliden/8231113/webrev.0
>>>
>>> /Per
>>