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

Roman Kennke rkennke at redhat.com
Mon Mar 19 08:47:06 UTC 2018

Am 17.03.2018 um 19:41 schrieb Kim Barrett:

On Mar 17, 2018, at 6:39 AM, Roman Kennke <rkennke at redhat.com> wrote:

Am 17.03.2018 um 05:11 schrieb Kim Barrett: On Mar 16, 2018, at 10:39 AM, Stefan Karlsson <stefan.karlsson at oracle.com> 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 ------------------------------------------------------------------------------ src/hotspot/share/classfile/javaClasses.cpp. 1870 address addr = ik->staticfieldaddr(staticunassignedstacktraceoffset); 1871 return HeapAccess<>::oopload((HeapWord*)addr); I'm not sure this is sufficient. Isn't staticfieldaddr just fundamentally broken for Shenandoah? It is not totally broken (I think addr() calls resolve() which kinda does what we need), Current implementation of staticfieldaddr is: address InstanceKlass::staticfieldaddr(int offset) { assert(offset >= InstanceMirrorKlass::offsetofstaticfields(), "has already been adjusted"); return (address)(offset + castfromoop(javamirror())); } javamirror() calls OopHandle::resolve(), which presently doesn’t do anything wrto Access (has OopHandle not yet been Accessorized? Or is there some reason why it’s okay as is that I’m not thinking of right now?) Even if it did use Access, I would expect for Shenandoah the access would just be a dereference of the oop* in the OopHandle, and would not chase the Brooks pointer, since we’re not accessing fields in the mirror object at that point.

Oh oops, I confused this with typeArrayOop::_addr(). Sorry. Yeah, we need appropriate access barriers in there too. I think it would be best if we either had static_field() and static_field_put() accessors in instanceKlass, or simply go vial ik->java_mirror()->_field() and ik->java_mirror()->*_field_put() ?

Roman

More information about the hotspot-dev mailing list