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

[Erik Österlund]

Hi Per,

Thanks for the review.

/Erik

On 2019-08-07 10:48, Per Liden wrote:
> 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
>>>