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

[coleen.phillimore at oracle.com]

On 3/19/18 3:15 PM, Stefan Karlsson wrote:
> 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?

I had to read this thread over again, and am still foggy, but it was 
because your original change didn't work for shenandoah, ie Kim's last 
response.

The brooks pointer has to be applied to get the mirror address as well 
as reading fields out of the mirror, if I understand correctly.

OopHandle::resolve() which is what java_mirror() is not accessorized but 
should be for shenandoah.  I think.  I guess that was my question before.

I agree it is best to avoid _addr() functions.

Coleen
>
>>
>> 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
>>>>>>
>>>
>>
>