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

Roman Kennke rkennke at redhat.com
Mon Mar 19 19:35:19 UTC 2018

Am 19.03.2018 um 20:15 schrieb Stefan Karlsson:

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.

Both should be covered by the Access::load* or store* calls.

Is that what you are referring to?

http://cr.openjdk.java.net/~rkennke/JDK-8199739/webrev.00/src/hotspot/share/oops/instanceKlass.hpp.udiff.html

+ oop staticfieldbase() { return javamirror(); } I wonder if this should be named staticfieldbaseraw to be consistent with recent changes to arrayOopDesc?: void* arrayOopDesc::base(BasicType type) const { oop resolvedobj = Access<>::resolve(asoop()); return arrayOop(resolvedobj)->baseraw(type); } void* arrayOopDesc::baseraw(BasicType type) const { return reinterpretcast<void*>(castfromoop(asoop()) + baseoffsetinbytes(type)); } Here base() has the barrier and the baseraw() doesn't have a barrier.

I don't actually want static_field_base() to have a barrier, because the HeapAccess<>::load_(oop_)at() already does the right thing. I would not rename it to static_field_base_raw() and/or add such a method.

In-fact, I'd prefer if arrayOopDesc::base() wouldn't have a barrier either, and all users do the right thing but that is another story (and RFE).

Roman

More information about the hotspot-dev mailing list