RFR (XXXS): 8175367: Wrong assert for UseCompressedOops in aarch64 Copy::conjoint_oops_atomic implementation

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR (XXXS): 8175367: Wrong assert for UseCompressedOops in aarch64 Copy::conjoint_oops_atomic implementation

Thomas Schatzl
Hi all,

  can I have quick reviews for this trivial change that removes an
assert that seems wrong for aarch64 code?

The aarch64 implementation of Copy::conjoint_oops_atomic contains an
assert(!UseCompressedOops,...) which may errorneously fire if the
method were used to copy oop arrays if it is true.
There does not seem to be a caller like this at the moment, i.e. the
code decides at a higher level whether to call this method or the
narrowoop overload depending on UseCompressedOops.

The other platform do not have this guard either, so I thought maybe it
is useful to remove this code before it asserts on the day a person
intentionally wants to copy regular oops (because he has some internal
buffer that stores regular oops) even when UseCompressedOops is true.

Originally this issue has been found while developing JDK-8159422 and
using that method to copy an internal oop buffer. Due to later review
changes, that call went away, so the fix got delayed.
Further, the main part of this change, that has originally been planned
for JDK-8166207, has been silently fixed when we open-sourced arm/arm64
port, with only this one-liner left in aarch64 code.

I thought it would be nice to have this fixed anyway.

CR:
https://bugs.openjdk.java.net/browse/JDK-8175367
Webrev:
http://cr.openjdk.java.net/~tschatzl/8175367/webrev/
Testing:
jprt (as mentioned, there is no applicable caller at this time)

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XXXS): 8175367: Wrong assert for UseCompressedOops in aarch64 Copy::conjoint_oops_atomic implementation

Andrew Haley
On 22/02/17 09:57, Thomas Schatzl wrote:
> Originally this issue has been found while developing JDK-8159422 and
> using that method to copy an internal oop buffer. Due to later review
> changes, that call went away, so the fix got delayed.
> Further, the main part of this change, that has originally been planned
> for JDK-8166207, has been silently fixed when we open-sourced arm/arm64
> port, with only this one-liner left in aarch64 code.
>
> I thought it would be nice to have this fixed anyway.

OK, thanks.  I agree.

Andrew.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (XXXS): 8175367: Wrong assert for UseCompressedOops in aarch64 Copy::conjoint_oops_atomic implementation

Thomas Schatzl
Hi Andrew,

On Wed, 2017-02-22 at 10:00 +0000, Andrew Haley wrote:

> On 22/02/17 09:57, Thomas Schatzl wrote:
> >
> > Originally this issue has been found while developing JDK-8159422
> > and using that method to copy an internal oop buffer. Due to later
> > review changes, that call went away, so the fix got delayed.
> > Further, the main part of this change, that has originally been
> > planned for JDK-8166207, has been silently fixed when we open-
> > sourced arm/arm64 port, with only this one-liner left in aarch64
> > code.
> >
> > I thought it would be nice to have this fixed anyway.
> OK, thanks.  I agree.
>

  thanks for your review.

Thomas

Loading...