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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Mon Mar 19 19:00:15 UTC 2018

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.

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 (?)

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

Currently java_mirror() has no load_barriers because it's an OopHandle in the ClassLoaderData.  Will it with Shenandoah and ZGC (even before concurrent class unloading?)

thanks, Coleen

On 3/19/18 9:24 AM, Roman Kennke wrote:

Hi Stefan,

To be honest, I'd find it better to get rid of staticfieldaddr() altogether, and I am also not a fan of resolve(): it seems easy to just cover everything with it, but for Shenandoah it means to do a write-barrier, even when a read-barrier would suffice (which is cheaper). I recognize that none of the affected code is very performance sensitive, but if there is a cleaner solution, I'd go for this. What about this approach: http://cr.openjdk.java.net/~rkennke/JDK-8199739/webrev.00/ ?

Roman

Hi again, Seems like Coleen wanted me to solve this problem slightly differently. She suggested that I add an Access<>::resolve barrier in staticfieldaddr: http://cr.openjdk.java.net/~stefank/8199739/webrev.03/ This will probably solve the primitive access for Shenandoah. What do you think? Thanks, StefanK On 2018-03-19 12:29, Stefan Karlsson wrote: Hi Roman,

On 2018-03-19 11:17, Roman Kennke wrote: Hi Stefan,

thank you! grepping for staticfieldaddr() yields some more places that'd need similar treatment: jlong javalangrefSoftReference::clock() void javalangrefSoftReference::setclock(jlong value) Maybe cover them as well? Or I'll file a separate issue. Your call. This seems like primitive accesses, I'll be happy to leave those to you. ;) Thanks, StefanK Thanks, Roman

Hi all, Kim and Roman commented that my patch doesn't work with Shenandoah. Here's an updated version: http://cr.openjdk.java.net/~stefank/8199739/webrev.02/ Thanks, StefanK On 2018-03-16 15:39, Stefan Karlsson wrote: Hi all, Please review this patch to use HeapAccess<>::oopload instead of oopDesc::loaddecodeheapoop when loading oops from static fields in javaClasses.cpp: http://cr.openjdk.java.net/~stefank/8199739/webrev.01/ https://bugs.openjdk.java.net/browse/JDK-8199739 It's necessary to use HeapAccess<>::oopload to inject load barriers for GCs that need them. Thanks, StefanK

More information about the hotspot-dev mailing list