RFR (S): 8191904: Refactor weak oops in ResolvedMethodTable to use the Access API

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

RFR (S): 8191904: Refactor weak oops in ResolvedMethodTable to use the Access API

Erik Österlund-2
Hi,

The ResolvedMethodTable has weak oop references in it. Currently it uses
explicit SATB enqueueing for G1 to make the weak semantics work.

This should be refactored to use the Access API instead. The previous
raw loads should be ON_PHANTOM_OOP | AS_NO_KEEPALIVE and the loads
followed by explicit G1 SATB enqueueing should be ON_PHANTOM_OOP.

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

Bug:
https://bugs.openjdk.java.net/browse/JDK-8191904

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

Re: RFR (S): 8191904: Refactor weak oops in ResolvedMethodTable to use the Access API

coleen.phillimore
This looks good.   I think we should make the literal() function in this
hashtable a pure virtual function and ShouldNotReachHere to not allow it
for these classes derived from oop.

BTW, should we expect a StringTable and ProtectionDomainEntryTable
change as well?  The latter is missing the SATB barrier at the moment.

Coleen

On 11/27/17 10:33 AM, Erik Österlund wrote:

> Hi,
>
> The ResolvedMethodTable has weak oop references in it. Currently it
> uses explicit SATB enqueueing for G1 to make the weak semantics work.
>
> This should be refactored to use the Access API instead. The previous
> raw loads should be ON_PHANTOM_OOP | AS_NO_KEEPALIVE and the loads
> followed by explicit G1 SATB enqueueing should be ON_PHANTOM_OOP.
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8191904/webrev.00/
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8191904
>
> Thanks,
> /Erik

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8191904: Refactor weak oops in ResolvedMethodTable to use the Access API

Kim Barrett
In reply to this post by Erik Österlund-2
> On Nov 27, 2017, at 10:33 AM, Erik Österlund <[hidden email]> wrote:
>
> Hi,
>
> The ResolvedMethodTable has weak oop references in it. Currently it uses explicit SATB enqueueing for G1 to make the weak semantics work.
>
> This should be refactored to use the Access API instead. The previous raw loads should be ON_PHANTOM_OOP | AS_NO_KEEPALIVE and the loads followed by explicit G1 SATB enqueueing should be ON_PHANTOM_OOP.
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8191904/webrev.00/
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8191904
>
> Thanks,
> /Erik

==============================================================================
src/hotspot/share/prims/resolvedMethodTable.cpp
  44   // The ACCESS_PEEK peeks at the oop without keeping it alive.

ACCESS_PEEK => AS_NO_KEEPALIVE.

==============================================================================
src/hotspot/share/prims/resolvedMethodTable.cpp
  43 oop ResolvedMethodEntry::object_peek() {

Maybe the function should be called object_no_keepalive(), for
consistency with the access decorator.

==============================================================================  
src/hotspot/share/prims/resolvedMethodTable.cpp
  43 oop ResolvedMethodEntry::object_peek() {
  44   // The ACCESS_PEEK peeks at the oop without keeping it alive.
  45   // This is dangerous in general but is okay in this specific
  46   // case. A subsequent oop_load keeps the oop alive if it it matched
  47   // a target object before leaking out.

I think the comment is misleading. There is nothing about this
specific function that makes it "okay"; what makes it okay is that
each use is careful about how it uses the result.

==============================================================================
src/hotspot/share/prims/resolvedMethodTable.cpp

Many (though not all) uses of object_peek occur in conjunction with a
call to vmtarget for the peeked object.  It might be worth adding a
helper for that case, to reduce the number of peek calls that need to
be monitored to ensure the peeked value doesn't escape.

Hm, but what happens to the vmtarget if the peeked object were to die?
If the vmtarget dies too (or might die too), then this might not help
after all.

==============================================================================

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8191904: Refactor weak oops in ResolvedMethodTable to use the Access API

Erik Österlund-2
Hi Kim,

Thank you for the review.

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

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

On 2017-11-28 01:33, Kim Barrett wrote:

>> On Nov 27, 2017, at 10:33 AM, Erik Österlund <[hidden email]> wrote:
>>
>> Hi,
>>
>> The ResolvedMethodTable has weak oop references in it. Currently it uses explicit SATB enqueueing for G1 to make the weak semantics work.
>>
>> This should be refactored to use the Access API instead. The previous raw loads should be ON_PHANTOM_OOP | AS_NO_KEEPALIVE and the loads followed by explicit G1 SATB enqueueing should be ON_PHANTOM_OOP.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8191904/webrev.00/
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8191904
>>
>> Thanks,
>> /Erik
> ==============================================================================
> src/hotspot/share/prims/resolvedMethodTable.cpp
>    44   // The ACCESS_PEEK peeks at the oop without keeping it alive.
>
> ACCESS_PEEK => AS_NO_KEEPALIVE.

Fixed.

> ==============================================================================
> src/hotspot/share/prims/resolvedMethodTable.cpp
>    43 oop ResolvedMethodEntry::object_peek() {
>
> Maybe the function should be called object_no_keepalive(), for
> consistency with the access decorator.

Fixed.

> ==============================================================================
> src/hotspot/share/prims/resolvedMethodTable.cpp
>    43 oop ResolvedMethodEntry::object_peek() {
>    44   // The ACCESS_PEEK peeks at the oop without keeping it alive.
>    45   // This is dangerous in general but is okay in this specific
>    46   // case. A subsequent oop_load keeps the oop alive if it it matched
>    47   // a target object before leaking out.
>
> I think the comment is misleading. There is nothing about this
> specific function that makes it "okay"; what makes it okay is that
> each use is careful about how it uses the result.

Fixed (hopefully).

> ==============================================================================
> src/hotspot/share/prims/resolvedMethodTable.cpp
>
> Many (though not all) uses of object_peek occur in conjunction with a
> call to vmtarget for the peeked object.  It might be worth adding a
> helper for that case, to reduce the number of peek calls that need to
> be monitored to ensure the peeked value doesn't escape.

I counted two cases where we from a "peeked" value only looked at
vmtarget and 3 cases where we did more than that. So it seemed like too
much to introduce an abstraction that "peeked" and fetched the vmtarget
for only two uses. Do you agree?

> Hm, but what happens to the vmtarget if the peeked object were to die?
> If the vmtarget dies too (or might die too), then this might not help
> after all.

That is true.

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

Re: RFR (S): 8191904: Refactor weak oops in ResolvedMethodTable to use the Access API

Erik Österlund-2
In reply to this post by coleen.phillimore
Hi Coleen,

On 2017-11-27 19:33, [hidden email] wrote:
> This looks good.

Thank you for the review.

> I think we should make the literal() function in this hashtable a pure
> virtual function and ShouldNotReachHere to not allow it for these
> classes derived from oop.

That might be a good idea indeed.

> BTW, should we expect a StringTable and ProtectionDomainEntryTable
> change as well?  The latter is missing the SATB barrier at the moment.

StringTable changes are coming up indeed. About the
ProtectionDomainEntryTable, I have not prepared a patch yet. From
looking at it briefly, it did look like we were only ever "peeking" at
the value and never exposing it. So should probably just use
RootAccess<ON_PHANTOM_OOP_REF | AS_NO_KEEPALIVE>::oop_load all over.
This will not translate to any actual barrier on any collector, but is
probably a good abstraction to have anyway. If you want this, I will
prepare a patch for it.

Thanks,
/Erik

> Coleen
>
> On 11/27/17 10:33 AM, Erik Österlund wrote:
>> Hi,
>>
>> The ResolvedMethodTable has weak oop references in it. Currently it
>> uses explicit SATB enqueueing for G1 to make the weak semantics work.
>>
>> This should be refactored to use the Access API instead. The previous
>> raw loads should be ON_PHANTOM_OOP | AS_NO_KEEPALIVE and the loads
>> followed by explicit G1 SATB enqueueing should be ON_PHANTOM_OOP.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8191904/webrev.00/
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8191904
>>
>> Thanks,
>> /Erik
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8191904: Refactor weak oops in ResolvedMethodTable to use the Access API

Thomas Schatzl
Hi,

On Tue, 2017-11-28 at 13:26 +0100, Erik Österlund wrote:

> Hi Coleen,
>
> On 2017-11-27 19:33, [hidden email] wrote:
> > This looks good.
>
> Thank you for the review.
>
> > I think we should make the literal() function in this hashtable a
> > pure virtual function and ShouldNotReachHere to not allow it for
> > these classes derived from oop.
>
> That might be a good idea indeed.
>
> > BTW, should we expect a StringTable and ProtectionDomainEntryTable
> > change as well?  The latter is missing the SATB barrier at the
> > moment.
>
> StringTable changes are coming up indeed. About the
> ProtectionDomainEntryTable, I have not prepared a patch yet. From
> looking at it briefly, it did look like we were only ever "peeking"
> at the value and never exposing it. So should probably just use
> RootAccess<ON_PHANTOM_OOP_REF | AS_NO_KEEPALIVE>::oop_load all over.
> This will not translate to any actual barrier on any collector, but
> is probably a good abstraction to have anyway. If you want this, I
> will prepare a patch for it.

If I understand its use correctly (still wrapping my head around this),
since some of your other changes also add this abstraction "where it is
not needed" it would be nice to have.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR (S): 8191904: Refactor weak oops in ResolvedMethodTable to use the Access API

Erik Österlund-2
Hi Thomas,

On 2017-11-28 14:12, Thomas Schatzl wrote:

> Hi,
>
> On Tue, 2017-11-28 at 13:26 +0100, Erik Österlund wrote:
>> Hi Coleen,
>>
>> On 2017-11-27 19:33, [hidden email] wrote:
>>> This looks good.
>> Thank you for the review.
>>
>>> I think we should make the literal() function in this hashtable a
>>> pure virtual function and ShouldNotReachHere to not allow it for
>>> these classes derived from oop.
>> That might be a good idea indeed.
>>
>>> BTW, should we expect a StringTable and ProtectionDomainEntryTable
>>> change as well?  The latter is missing the SATB barrier at the
>>> moment.
>> StringTable changes are coming up indeed. About the
>> ProtectionDomainEntryTable, I have not prepared a patch yet. From
>> looking at it briefly, it did look like we were only ever "peeking"
>> at the value and never exposing it. So should probably just use
>> RootAccess<ON_PHANTOM_OOP_REF | AS_NO_KEEPALIVE>::oop_load all over.
>> This will not translate to any actual barrier on any collector, but
>> is probably a good abstraction to have anyway. If you want this, I
>> will prepare a patch for it.
> If I understand its use correctly (still wrapping my head around this),
> since some of your other changes also add this abstraction "where it is
> not needed" it would be nice to have.

Okay, will sort out.

Thanks,
/Erik

> Thanks,
>    Thomas
>