RFR: 8199739: Use HeapAccess when loading oops from static fields in javaClasses.cpp

[coleen.phillimore at oracle.com]

I like Roman's version with static_field_base() the best.  The reason I 
wanted to keep static_field_addr and not have static_oop_addr was so 
there is one function to find static fields and this would work with the 
jvmci classes and with loading/storing primitives also.  So I like the 
consistent change that Roman has.

There's a subtlety that I haven't quite figured out here. 
static_field_addr gets an address mirror+offset, so needs a load barrier 
on this offset, then needs a load barrier on the offset of the 
additional load (?)

http://cr.openjdk.java.net/~rkennke/JDK-8199739/webrev.00/src/hotspot/share/oops/instanceKlass.hpp.udiff.html

+ oop static_field_base() { return java_mirror(); }


Currently java_mirror() has no load_barriers because it's an OopHandle 
in the ClassLoaderData.  Will it with Shenandoah and ZGC (even before 
concurrent class unloading?)

thanks,
Coleen

On 3/19/18 9:24 AM, Roman Kennke wrote:
> Hi Stefan,
>
> To be honest, I'd find it better to get rid of static_field_addr()
> altogether, and I am also not a fan of resolve(): it seems easy to just
> cover everything with it, but for Shenandoah it means to do a
> write-barrier, even when a read-barrier would suffice (which is
> cheaper). I recognize that none of the affected code is very performance
> sensitive, but if there is a cleaner solution, I'd go for this. What
> about this approach:
>
> http://cr.openjdk.java.net/~rkennke/JDK-8199739/webrev.00/
>
> ?
>
>
> Roman
>
>
>> Hi again,
>>
>> Seems like Coleen wanted me to solve this problem slightly differently.
>> She suggested that I add an Access<>::resolve barrier in static_field_addr:
>>
>> http://cr.openjdk.java.net/~stefank/8199739/webrev.03/
>>
>> This will probably solve the primitive access for Shenandoah. What do
>> you think?
>>
>> Thanks,
>> StefanK
>>
>> On 2018-03-19 12:29, Stefan Karlsson wrote:
>>> Hi Roman,
>>>
>>> On 2018-03-19 11:17, Roman Kennke wrote:
>>>> Hi Stefan,
>>>>
>>>> thank you!
>>>>
>>>> grepping for static_field_addr() yields some more places that'd need
>>>> similar treatment:
>>>>
>>>> jlong java_lang_ref_SoftReference::clock()
>>>> void java_lang_ref_SoftReference::set_clock(jlong value)
>>>>
>>>> Maybe cover them as well? Or I'll file a separate issue. Your call.
>>> This seems like primitive accesses, I'll be happy to leave those to
>>> you. ;)
>>>
>>> Thanks,
>>> StefanK
>>>
>>>> Thanks, Roman
>>>>
>>>>
>>>>> Hi all,
>>>>>
>>>>> Kim and Roman commented that my patch doesn't work with Shenandoah.
>>>>> Here's an updated version:
>>>>> http://cr.openjdk.java.net/~stefank/8199739/webrev.02/
>>>>>
>>>>> Thanks,
>>>>> StefanK
>>>>>
>>>>> On 2018-03-16 15:39, Stefan Karlsson wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Please review this patch to use HeapAccess<>::oop_load instead of
>>>>>> oopDesc::load_decode_heap_oop when loading oops from static fields in
>>>>>> javaClasses.cpp:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~stefank/8199739/webrev.01/
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8199739
>>>>>>
>>>>>> It's necessary to use HeapAccess<>::oop_load to inject load barriers
>>>>>> for GCs that need them.
>>>>>>
>>>>>> Thanks,
>>>>>> StefanK
>>>>
>