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

[Stefan Karlsson]

On 2018-03-19 21:11, Roman Kennke wrote:
> 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?

I assigned the bug to you.

Cheers,
StefanK
>
> Roman
>