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

[Roman Kennke]

Am 20.03.2018 um 11:44 schrieb Erik Österlund:
> Hi Roman,
> 
> On 2018-03-20 11:26, Roman Kennke wrote:
>> Am 20.03.2018 um 11:07 schrieb 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?
>> No, I don't think that resolve() can go away. If you look at:
>>
>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2018-March/021464.html
>>
>>
>> You'll see all kinds of uses of _at_addr() that cannot be covered by
>> some sort of arraycopy, e.g. the string conversions stuff.
>>
>> The above patch proposes to split resolve() to resolve_for_read() and
>> resolve_for_write(), and I don't think it is unreasonable to distinguish
>> those. Besides being better for Shenandoah (reduced latency on read-only
>> accesses), there are conceivable GC algorithms that require that
>> distinction too, e.g. transactional memory based GC or copy-on-write
>> based GCs. But let's probably continue this discussion in the thread
>> mentioned above?
> 
> As I thought. The reason I bring it up in this thread is because as I
> understand it, you are proposing to push this patch without renaming
> static_field_base() to static_field_base_raw(), which is what we did
> consistently everywhere else so far, with the motivation that you will
> remove resolve() from the other ones soon, and get rid of base_raw().
> And I feel like we should have that discussion first. Until that is
> actually changed, static_field_base_raw() should be the name of that
> method. If we decide to change the other code to do something else, then
> we can revisit this then, but not yet.

Ok, so I changed static_field_base() -> static_field_base_raw():

Diff:
http://cr.openjdk.java.net/~rkennke/JDK-8199739/webrev.01.diff/
Full:
http://cr.openjdk.java.net/~rkennke/JDK-8199739/webrev.01/

Better?

Thanks, Roman