RFR: 8199739: Use HeapAccess when loading oops from static fields in javaClasses.cpp
Stefan Karlsson
stefan.karlsson at oracle.com
Thu Mar 22 10:46:29 UTC 2018
On 2018-03-22 11:40, Roman Kennke wrote:
> I keep getting:
>
> Mach5 mach5-one-rkennke-JDK-8199739-20180322-0911-15544: Builds
> UNSTABLE. Testing UNSTABLE.
>
> Do I need to be worried?
Yes.
In a closed file you get:
fatal error: runtime/vframe.inline.hpp: No such file or directory
I think that this will be resolved if you rebase against the latest open
changes.
StefanK
>
> Full output:
>
> Build Details: 2018-03-22-0901391.roman.source
> 0 Failed Tests
> Mach5 Tasks Results Summary
>
> PASSED: 3
> EXECUTED_WITH_FAILURE: 8
> UNABLE_TO_RUN: 64
> FAILED: 0
> KILLED: 0
> NA: 0
> Build
>
> 8 Not run
> build_jdk_linux-linux-x64-linux-x64-OEL-7-0 error while
> building, return value: 2
> build_jdk_linux-linux-x64-debug-linux-x64-OEL-7-1 error
> while building, return value: 2
> build_jdk_macosx-macosx-x64-macosx-x64-anyof_10_9_10_10-2
> error while building, return value: 2
>
> build_jdk_macosx-macosx-x64-debug-macosx-x64-anyof_10_9_10_10-3 error
> while building, return value: 2
> build_jdk_solaris-solaris-sparcv9-solaris-sparcv9-11_2-4
> error while building, return value: 2
>
> build_jdk_solaris-solaris-sparcv9-debug-solaris-sparcv9-11_2-5 error
> while building, return value: 2
> build_jdk_windows-windows-x64-windows-x64-2012R2-6 error
> while building, return value: 2
> build_jdk_windows-windows-x64-debug-windows-x64-2012R2-7
> error while building, return value: 2
>
> Test
>
> 64 Not run
>
> tier1-product-jdk_closed_test_hotspot_jtreg_tier1_common-linux-x64-15
> Dependency task failed:
> mach5...4-build_jdk_linux-linux-x64-linux-x64-OEL-7-0
>
> tier1-debug-jdk_closed_test_hotspot_jtreg_tier1_common-linux-x64-debug-48
> Dependency task failed:
> mach5...d_jdk_linux-linux-x64-debug-linux-x64-OEL-7-1
>
> tier1-product-jdk_closed_test_hotspot_jtreg_tier1_common-macosx-x64-16
> Dependency task failed:
> mach5...cosx-macosx-x64-macosx-x64-anyof_10_9_10_10-2
>
> tier1-debug-jdk_closed_test_hotspot_jtreg_tier1_common-macosx-x64-debug-49
> Dependency task failed:
> mach5...acosx-x64-debug-macosx-x64-anyof_10_9_10_10-3
>
> tier1-solaris-sparc-jdk_closed_test_hotspot_jtreg_tier1_common-solaris-sparcv9-62
> Dependency task failed:
> mach5...olaris-solaris-sparcv9-solaris-sparcv9-11_2-4
>
> tier1-solaris-sparc-jdk_closed_test_hotspot_jtreg_tier1_common-solaris-sparcv9-debug-63
> Dependency task failed:
> mach5...-solaris-sparcv9-debug-solaris-sparcv9-11_2-5
>
> tier1-product-jdk_closed_test_hotspot_jtreg_tier1_common-windows-x64-17
> Dependency task failed: viI3aXAomf
>
> tier1-debug-jdk_closed_test_hotspot_jtreg_tier1_common-windows-x64-debug-50
> Dependency task failed: viI3aXApmf
>
> tier1-debug-jdk_closed_test_hotspot_jtreg_tier1_compiler_closed-linux-x64-debug-51
> Dependency task failed:
> mach5...d_jdk_linux-linux-x64-debug-linux-x64-OEL-7-1
>
> tier1-debug-jdk_closed_test_hotspot_jtreg_tier1_compiler_closed-macosx-x64-debug-52
> Dependency task failed:
> mach5...acosx-x64-debug-macosx-x64-anyof_10_9_10_10-3
> See all 64...
>
>
>
>
>> 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
>>>>>
>>>>>
>>>
>>
>
>
More information about the hotspot-dev
mailing list