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

[Stefan Karlsson]

On 2018-03-19 20:00, coleen.phillimore at oracle.com wrote:
>
> 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.

That's OK with me. This RFE grew in scope of what I first intended, so 
I'm fine with Roman taking over this.

>
> 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 (?)
There are two barriers in this piece of code:
1) Shenandoah needs a barrier to be able to read fields out of the java 
mirror
2) ZGC and UseCompressedOops needs a barrier when loading oop fields in 
the java mirror.

Is that what you are referring to?

>
> 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(); }

I wonder if this should be named static_field_base_raw to be consistent 
with recent changes to arrayOopDesc?:

void* arrayOopDesc::base(BasicType type) const {
   oop resolved_obj = Access<>::resolve(as_oop());
   return arrayOop(resolved_obj)->base_raw(type);
}

void* arrayOopDesc::base_raw(BasicType type) const {
   return reinterpret_cast<void*>(cast_from_oop<intptr_t>(as_oop()) + 
base_offset_in_bytes(type));
}

Here base() has the barrier and the base_raw() doesn't have a barrier.

Thanks,
Stefank
>
>
> 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
>>>>>
>>
>