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

[Roman Kennke]

Am 19.03.2018 um 20:15 schrieb 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.

Both should be covered by the Access::load* or store* calls.

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

I don't actually want static_field_base() to have a barrier, because the
HeapAccess<>::load_(oop_)at() already does the right thing. I would not
rename it to static_field_base_raw() and/or add such a method.

In-fact, I'd prefer if arrayOopDesc::base() wouldn't have a barrier
either, and all users do the right thing but that is another story (and
RFE).

Roman