RFR: 8229027: Improve how JNIHandleBlock::oops_do distinguishes oops from non-oops

[Per Liden]

Hi Erik,

On 8/7/19 8:53 AM, Erik Österlund wrote:
> Hi Dean,
> 
> Thanks for having a look at this code.
> 
> Yes indeed. I actually thought about using union, but decided not to in 
> the end. Here is why: if sizeof(oop) != sizeof(uintptr_t), then we would 
> already be in a world of trouble as there is already so much code that 
> relies on that, which would fundamentally break, that I think it's fair 
> to assume such a change is very unlikely to happen.
> 
> Here are a few examples of things that would be unfortunate if 
> sizeof(oop) != sizeof(uintptr_t):
> 
> * All GCs use OopClosures, requiring oops in Java objects in the heap to 
> be oops. But we need to be able to use Atomic on oops. So they can't be 
> larger than uintptr_t.
> * Having a different object layout in debug and release would be very 
> unfortunate in general, because we would be testing something different 
> to what a release build looks like.
> * The JIT and a lot of platform dependent hand written assembly code 
> would have to change to deal with reading wide oops from the heap, and 
> all hardcoded assumptions everywhere else in e.g. compilers that oops 
> are pointers would probably not work.
> 
> However, having said that, I agree that it would be a good idea to at 
> least make the assumption made more clear. I inserted a STATIC_ASSERT 
> that checks the sizes, with an appropriate comment explaining that if 
> you are changing sizeof(oop), then you have to change this freelist 
> handling. At least this way, if someone eventually wants to change this 
> assumption, they would notice this very explicitly as you could not 
> compile without knowing this code must be changed.
> 
> What do you think? If you still think using union is better, then I am 
> open to doing that as well, and do not have strong opinions about it.
> 
> Incremental:
> http://cr.openjdk.java.net/~eosterlund/8229027/webrev.00_01/
> 
> Full:
> http://cr.openjdk.java.net/~eosterlund/8229027/webrev.01/

Looks good to me. I personally think the static assert is good enough, 
for the reasons you stated.

Minor nit, it looks like this comment now ended up inside the if 
statement instead of before it.

482         if (value != 0 && !is_tagged_free_list(value)) {
483           // traverse heap pointers only, not deleted handles or 
free list
484           // pointers

No need to see a new webrev.

cheers,
Per

> 
> Thanks,
> /Erik
> 
> On 2019-08-07 07:30, dean.long at oracle.com wrote:
>> Hi Erik.  Doesn't this only work if sizeof (class oop) <= sizeof 
>> (uintptr_t)?  If a compiler uses more space for class oop, or we add a 
>> debug field, then it will break.  Wouldn't a union be safer?
>>
>> dl
>>
>> On 8/6/19 6:04 AM, Erik Österlund wrote:
>>> Hi,
>>>
>>> In JNIHandleBlock::oops_do(), the blocks may contain free list next 
>>> pointers or oops. The distinction is detected by asking the 
>>> CollectedHeap if the pointer value is in the heap reserve or not.
>>> This forces GCs to have one single contiguous heap reservation, 
>>> without holes in it, which seems like an unnecessary restriction (and 
>>> effectively blocks a ZGC mac port).
>>>
>>> This patch removes this reliance by tagging the free list next 
>>> pointers instead, allowing the distinction between oops and freelist 
>>> pointers based on the low order bit value.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8229027
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8229027/webrev.00/
>>>
>>>
>>> Thanks,
>>> /Erik
>>