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

[Erik Österlund]

Hi Thomas,

Thanks for the review!

/Erik

On 2019-08-07 09:26, Thomas Stüfe wrote:
> Hi Erik,
> 
> This looks good to me. I think it may also be more efficient since you 
> have better locality. I have no strong feelings wrt using a union.
> 
> Cheers, Thomas.
> 
> On Wed 7. Aug 2019 at 08:54, Erik Österlund <erik.osterlund at oracle.com 
> <mailto:erik.osterlund at oracle.com>> 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/
> 
>     Thanks,
>     /Erik
> 
>     On 2019-08-07 07:30, dean.long at oracle.com
>     <mailto: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
>      >
>