RFR: 8199739: Use HeapAccess when loading oops from static fields in javaClasses.cpp (original) (raw)

Roman Kennke rkennke at redhat.com
Wed Mar 21 14:28:18 UTC 2018

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 staticfieldbase() the best.  The reason I wanted to keep staticfieldaddr and not have staticoopaddr 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. staticfieldaddr 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 javamirror() 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::*ataddr() 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 ataddr() 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 ataddr() that cannot be covered by some sort of arraycopy, e.g. the string conversions stuff. The above patch proposes to split resolve() to resolveforread() and resolveforwrite(), 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 staticfieldbase() to staticfieldbaseraw(), 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 baseraw(). And I feel like we should have that discussion first. Until that is actually changed, staticfieldbaseraw() 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 staticfieldbase() -> staticfieldbaseraw(): 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