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

[Roman Kennke]

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

The family of _at() functions in Access, those which accept oop+offset,
do the chasing of the forwarding pointer in Shenandoah, then they apply
the offset, load the memory field and return the value in the right
type. They also do the load-barrier in ZGC (haven't checked, but that's
just logical).

There is also oop Access::resolve(oop) which is a bit of a hack. It has
been introduced because of arraycopy and java <-> native bulk copy stuff
that uses typeArrayOop::*_at_addr() family of methods. In those
situations we still need to 1. chase the fwd ptr (for reads) or 2. maybe
evacuate the object (for writes), where #2 is stronger than #1 (i.e. if
we do #2, then we don't need to do #1). In order to keep things simple,
we decided to make Access::resolve(oop) do #2, and have it cover all
those cases, and put it in arrayOopDesc::base(). This does the right
thing for all cases, but it is a bit broad, for example, it may lead to
double-copying a potentially large array (resolve-copy src array from
from-space to to-space, then copy it again to the dst array). For those
reasons, it is advisable to think twice before using _at_addr() or
in-fact Access::resolve() if there's a better/cleaner way to do it.

Stefan: Should I assign the bug to me and take it over? Or do you want
to take my patch and push it yourself. I don't mind either way?

Roman