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

[Erik Österlund]

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 <erik.osterlund at oracle.com> 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