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

[Erik Österlund]

Hi Roman,

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.

Are we certain that it is indeed only arraycopy that requires stable 
accesses until the next thread transition?
I seem to recall that last time we discussed this, you thought that 
there was more than arraycopy code that needed this. For example 
printing and string encoding/decoding logic.

If we are going to make changes based on the assumption that we will be 
able to get rid of the resolve() barrier, then we should be fairly 
certain that we can indeed get rid of it. So have the other previously 
discussed roadblocks other than arraycopy disappeared?

Thanks,
/Erik

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