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

[Stefan Karlsson]

On 2018-03-19 20:35, Roman Kennke wrote:
> 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.

Yes.

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

>   I would not
> rename it to static_field_base_raw() and/or add such a method.

That makes this code inconsistent with the naming in arrayOopDesc. I 
don't find that very appealing.

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

Maybe this contention point needs to be resolved sooner rather than later.

StefanK

>
> Roman
>
>