RFR (S): 8193063: Enabling narrowOop values for RawAccess accesses

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR (S): 8193063: Enabling narrowOop values for RawAccess accesses

Erik Österlund-2
Hi,

In order to replace oopDesc::load_heap_oop() but with stricter memory
ordering properties, like MO_VOLATILE, some Access tweaks are required
to allow RawAccess<>::oop_load() to return narrowOop values.

I made the necessary changes to allow narrowOop values for all RawAccess
operations that have an address (not the _at variants).

While I am at it, I thought I'd clean up a few things that bug me:

* The decorator verification for memory ordering specifically did not
work as I originally intended, leading to harder to decipher compiler
errors deeper down when using the wrong memory ordering decorators
* An unnecessary include of oop.inline.hpp was removed from the G1
barrier set.

This change helps solving the following bug:
https://bugs.openjdk.java.net/browse/JDK-8129440

This bug is filed under:
https://bugs.openjdk.java.net/browse/JDK-8193063

Webrev:
http://cr.openjdk.java.net/~eosterlund/8193063/webrev.00/

Thanks,
/Erik
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8193063: Enabling narrowOop values for RawAccess accesses

Kim Barrett
> On Dec 5, 2017, at 9:49 AM, Erik Österlund <[hidden email]> wrote:
>
> Hi,
>
> In order to replace oopDesc::load_heap_oop() but with stricter memory ordering properties, like MO_VOLATILE, some Access tweaks are required to allow RawAccess<>::oop_load() to return narrowOop values.
>
> I made the necessary changes to allow narrowOop values for all RawAccess operations that have an address (not the _at variants).
>
> While I am at it, I thought I'd clean up a few things that bug me:
>
> * The decorator verification for memory ordering specifically did not work as I originally intended, leading to harder to decipher compiler errors deeper down when using the wrong memory ordering decorators

I kind of wish that had been dealt with separately.  Oh well.

> * An unnecessary include of oop.inline.hpp was removed from the G1 barrier set.
>
> This change helps solving the following bug:
> https://bugs.openjdk.java.net/browse/JDK-8129440
>
> This bug is filed under:
> https://bugs.openjdk.java.net/browse/JDK-8193063
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8193063/webrev.00/
>
> Thanks,
> /Erik

------------------------------------------------------------------------------
src/hotspot/share/oops/access.inline.hpp
 525       typedef RawAccessBarrier<decorators & RAW_DECORATOR_MASK> Raw;
 582       typedef RawAccessBarrier<decorators & RAW_DECORATOR_MASK> Raw;

I don't see any use of these Raw types. They look like copy-paste as
part of splitting into two specializations.

------------------------------------------------------------------------------
src/hotspot/share/oops/access.inline.hpp

Too bad we don't have C++17. I think making can_hardwire_raw constexpr
and using the new "if constexpr" syntax would have been sufficient.

------------------------------------------------------------------------------
src/hotspot/share/oops/access.inline.hpp

I was initially thinking I wanted names for these two expressions,
since they are repeated a bunch of times:

HasDecorator<decorators, AS_RAW>::value && CanHardwireRaw<decorators>::value
HasDecorator<decorators, AS_RAW>::value && !CanHardwireRaw<decorators>::value

But I think what I really want is the better function template SFINAE
syntax allowed by C++11, e.g.

  template<DecoratorSet decorators, typename T,
           ENABLE_IF(HasDecorator<Decorators, AS_RAW>),
           DISABLE_IF(CanHardwireRaw<decorators>)>
  inline static T store(...) ...

------------------------------------------------------------------------------
src/hotspot/share/oops/access.inline.hpp
 796   // Step 2: Reduce types.

This comment looks like it needs updating for the narrowOop value
support.

------------------------------------------------------------------------------
src/hotspot/share/oops/accessBackend.hpp
 176   AccessInternal::MustConvertCompressedOop<idecorators, T>::value,

Wrong indentation.

------------------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8193063: Enabling narrowOop values for RawAccess accesses

Per Liden
In reply to this post by Erik Österlund-2
Looks good!

/Per

On 2017-12-05 15:49, Erik Österlund wrote:

> Hi,
>
> In order to replace oopDesc::load_heap_oop() but with stricter memory
> ordering properties, like MO_VOLATILE, some Access tweaks are required
> to allow RawAccess<>::oop_load() to return narrowOop values.
>
> I made the necessary changes to allow narrowOop values for all RawAccess
> operations that have an address (not the _at variants).
>
> While I am at it, I thought I'd clean up a few things that bug me:
>
> * The decorator verification for memory ordering specifically did not
> work as I originally intended, leading to harder to decipher compiler
> errors deeper down when using the wrong memory ordering decorators
> * An unnecessary include of oop.inline.hpp was removed from the G1
> barrier set.
>
> This change helps solving the following bug:
> https://bugs.openjdk.java.net/browse/JDK-8129440
>
> This bug is filed under:
> https://bugs.openjdk.java.net/browse/JDK-8193063
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8193063/webrev.00/
>
> Thanks,
> /Erik
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8193063: Enabling narrowOop values for RawAccess accesses

Erik Österlund-2
In reply to this post by Kim Barrett
Hi Kim,

Thank you for the review.

Incremental webrev:
http://cr.openjdk.java.net/~eosterlund/8193063/webrev.00_01/

Full webrev:
http://cr.openjdk.java.net/~eosterlund/8193063/webrev.01/

On 2018-01-08 20:23, Kim Barrett wrote:

>> On Dec 5, 2017, at 9:49 AM, Erik Österlund <[hidden email]> wrote:
>>
>> Hi,
>>
>> In order to replace oopDesc::load_heap_oop() but with stricter memory ordering properties, like MO_VOLATILE, some Access tweaks are required to allow RawAccess<>::oop_load() to return narrowOop values.
>>
>> I made the necessary changes to allow narrowOop values for all RawAccess operations that have an address (not the _at variants).
>>
>> While I am at it, I thought I'd clean up a few things that bug me:
>>
>> * The decorator verification for memory ordering specifically did not work as I originally intended, leading to harder to decipher compiler errors deeper down when using the wrong memory ordering decorators
> I kind of wish that had been dealt with separately.  Oh well.
>
>> * An unnecessary include of oop.inline.hpp was removed from the G1 barrier set.
>>
>> This change helps solving the following bug:
>> https://bugs.openjdk.java.net/browse/JDK-8129440
>>
>> This bug is filed under:
>> https://bugs.openjdk.java.net/browse/JDK-8193063
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8193063/webrev.00/
>>
>> Thanks,
>> /Erik
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/access.inline.hpp
>   525       typedef RawAccessBarrier<decorators & RAW_DECORATOR_MASK> Raw;
>   582       typedef RawAccessBarrier<decorators & RAW_DECORATOR_MASK> Raw;
>
> I don't see any use of these Raw types. They look like copy-paste as
> part of splitting into two specializations.

Fixed.

>
> ------------------------------------------------------------------------------
> src/hotspot/share/oops/access.inline.hpp
>
> Too bad we don't have C++17. I think making can_hardwire_raw constexpr
> and using the new "if constexpr" syntax would have been sufficient.

Indeed. I stopped for a second, cried a bit about the lack of constexpr,
and then did it the C++03 way.

> ------------------------------------------------------------------------------
> src/hotspot/share/oops/access.inline.hpp
>
> I was initially thinking I wanted names for these two expressions,
> since they are repeated a bunch of times:
>
> HasDecorator<decorators, AS_RAW>::value && CanHardwireRaw<decorators>::value
> HasDecorator<decorators, AS_RAW>::value && !CanHardwireRaw<decorators>::value
>
> But I think what I really want is the better function template SFINAE
> syntax allowed by C++11, e.g.
>
>    template<DecoratorSet decorators, typename T,
>             ENABLE_IF(HasDecorator<Decorators, AS_RAW>),
>             DISABLE_IF(CanHardwireRaw<decorators>)>
>    inline static T store(...) ...

Yes, we are a bit stuck with old school SFINAE for some time.

> ------------------------------------------------------------------------------
> src/hotspot/share/oops/access.inline.hpp
>   796   // Step 2: Reduce types.
>
> This comment looks like it needs updating for the narrowOop value
> support.

Unfortunately, the comment was already accurate (and hence was
inaccurate before).

> ------------------------------------------------------------------------------
> src/hotspot/share/oops/accessBackend.hpp
>   176   AccessInternal::MustConvertCompressedOop<idecorators, T>::value,
>
> Wrong indentation.
>
> ------------------------------------------------------------------------------

Fixed.

Thanks,
/Erik
Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8193063: Enabling narrowOop values for RawAccess accesses

Erik Österlund-2
In reply to this post by Per Liden
Hi Per,

Thank you for the review.

/Erik

On 2018-01-09 11:18, Per Liden wrote:

> Looks good!
>
> /Per
>
> On 2017-12-05 15:49, Erik Österlund wrote:
>> Hi,
>>
>> In order to replace oopDesc::load_heap_oop() but with stricter memory
>> ordering properties, like MO_VOLATILE, some Access tweaks are required
>> to allow RawAccess<>::oop_load() to return narrowOop values.
>>
>> I made the necessary changes to allow narrowOop values for all RawAccess
>> operations that have an address (not the _at variants).
>>
>> While I am at it, I thought I'd clean up a few things that bug me:
>>
>> * The decorator verification for memory ordering specifically did not
>> work as I originally intended, leading to harder to decipher compiler
>> errors deeper down when using the wrong memory ordering decorators
>> * An unnecessary include of oop.inline.hpp was removed from the G1
>> barrier set.
>>
>> This change helps solving the following bug:
>> https://bugs.openjdk.java.net/browse/JDK-8129440
>>
>> This bug is filed under:
>> https://bugs.openjdk.java.net/browse/JDK-8193063
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8193063/webrev.00/
>>
>> Thanks,
>> /Erik

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8193063: Enabling narrowOop values for RawAccess accesses

Kim Barrett
In reply to this post by Erik Österlund-2
> On Jan 9, 2018, at 6:09 AM, Erik Österlund <[hidden email]> wrote:
>
> Hi Kim,
>
> Thank you for the review.
>
> Incremental webrev:
> http://cr.openjdk.java.net/~eosterlund/8193063/webrev.00_01/
>
> Full webrev:
> http://cr.openjdk.java.net/~eosterlund/8193063/webrev.01/

Looks good.

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8193063: Enabling narrowOop values for RawAccess accesses

Erik Österlund-2
Hi Kim,

Thank you for the review.

/Erik

On 2018-01-09 18:14, Kim Barrett wrote:

>> On Jan 9, 2018, at 6:09 AM, Erik Österlund <[hidden email]> wrote:
>>
>> Hi Kim,
>>
>> Thank you for the review.
>>
>> Incremental webrev:
>> http://cr.openjdk.java.net/~eosterlund/8193063/webrev.00_01/
>>
>> Full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8193063/webrev.01/
> Looks good.
>