RFR: 8199739: Use HeapAccess when loading oops from static fields in javaClasses.cpp
Roman Kennke
rkennke at redhat.com
Thu Mar 22 10:40:27 UTC 2018
I keep getting:
Mach5 mach5-one-rkennke-JDK-8199739-20180322-0911-15544: Builds
UNSTABLE. Testing UNSTABLE.
Do I need to be worried?
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