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

Wed Mar 21 14:39:57 UTC 2018

Hi Roman,

I got the same problem when pushing the remove Runtime1::arraycopy 
changes, so I can confirm this is unrelated to your changes.

Thanks,
/Erik

On 2018-03-21 15:28, Roman Kennke wrote:
> I got a failure back from submit repo:
>
> Build Details: 2018-03-21-1213342.roman.source
> 1 Failed Test
> Test	Tier	Platform	Keywords	Description	Task
> compiler/jsr292/RedefineMethodUsedByMultipleMethodHandles.java 	tier1
> macosx-x64-debug 	bug8042235 othervm 	Exception:
> java.lang.reflect.InvocationTargetException 	task
> Mach5 Tasks Results Summary
>
>      PASSED: 74
>      EXECUTED_WITH_FAILURE: 1
>      UNABLE_TO_RUN: 0
>      FAILED: 0
>      KILLED: 0
>      NA: 0
>      Test
>
>          1 Executed with failure
>
> tier1-debug-jdk_open_test_hotspot_jtreg_tier1_compiler_3-macosx-x64-debug-28
> Results: total: 165, passed: 164; failed: 1
>
>
> Can you tell if that is related to the change, or something other
> already known issue?
>
> Thanks, Roman
>
>
>> Hi Roman,
>>
>> This looks good to me. The unfortunate include problems in
>> jvmciJavaClasses.hpp are pre-existing and should be cleaned up at some
>> point.
>>
>> Thanks,
>> /Erik
>>
>> On 2018-03-20 16:13, Roman Kennke wrote:
>>> 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
>>>
>>>
>