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

Stefan Karlsson stefan.karlsson at oracle.com
Mon Mar 19 20:23:11 UTC 2018

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. Stefan: Should I assign the bug to me and take it over? Or do you want to take my patch and push it yourself. I don't mind either way?

I assigned the bug to you.

Cheers, StefanK

Roman

More information about the hotspot-dev mailing list