RFR[L]: 8237767 Field layout computation overhaul

Tue Jan 28 14:09:06 UTC 2020

Coleen,

Thank you for reviewing this change.

New webrev:
http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.06/index.html

My answers are inlined below.

> On Jan 24, 2020, at 16:17, coleen.phillimore at oracle.com wrote:
> 
> 
> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/ci/ciInstanceKlass.hpp.udiff.html
> 
>   bool contains_field_offset(int offset) {
> - return instanceOopDesc::contains_field_offset(offset, nonstatic_field_size());
> + fieldDescriptor fd;
> + return this->get_instanceKlass()->find_field_from_offset(offset, false, &fd);
>   }
> 
> 
> This has to go into the VM if it's going to access metadata, with VM_ENTRY_MARK, so probably belongs in the cpp file. Also, why doesn't this call contains_field_offset() from InstanceKlass? from here:
> 
> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/oops/instanceKlass.cpp.udiff.html
> 
> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/oops/instanceKlass.hpp.udiff.html

Good catch! I’ve changed the code to:

bool ciInstanceKlass::contains_field_offset(int offset) {
  VM_ENTRY_MARK;
  return get_instanceKlass()->contains_field_offset(offset);
}

> 
> + public:
> + enum {
> + _extra_is_being_redefined = 1 << 0, // used for locking redefinition
> + _extra_has_contended_annotations = 1 << 1 // has @Contended annotation
> + };
> +
> 
> 
> Why is this enum public?  Also, I think you should make these misc_flags and make _flags a u4.  There's already an alignment gap in InstanceKlass and we can file an RFE to fix that after this change.

I’ve extended _misc_flags to a u4 and moved all flags to it.
Note: the type change impacts vmStruct and JVMCI.

> 
> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/runtime/globals.hpp.udiff.html
> 
> I think you should make UseNewLayout a diagnostic flag since we would like to remove the old code once it has gotten more testing. The only reason someone would use it would be to diagnose some differing behavior.

Done.

> 
> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotResolvedObjectTypeImpl.java.udiff.html
> 
> Someone who knows the compilers should have a peek at this.
> 
> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/classfile/fieldLayoutBuilder.hpp.html
> 
>  37 // A RawBlock describes an element of a layout.
>  38 // Each field is represented by a RawBlock.
> 
> Thank you for changing RawBlock to LayoutRawBlock.   Can you update the comments here and the description in the RFE?

Comments and RFE fixed.

> 
>  29 #include "classfile/classLoaderData.inline.hpp"
> 
> 
> .hpp files shouldn't include .inline.hpp files.  Whatever uses the inline code should go into the cpp file (or if critical, add an inline.hpp file).

Fixed.

> 
> 132   static const int INITIAL_LIST_SIZE;
> 
> 
> It's odd to see this without an initialization.

I moved the initialization from the .cpp to the .hpp file.

> 
> 227 class FieldLayoutBuilder : public ResourceObj {
> 
> 
> FieldLayoutBuilder is a StackObj isn't it?
> 
> It might be nice to line up the fields, which is part of the coding standard that I only somewhat agree on.  Here it would enhance readability.

Done.

> 
> 264   protected:
> 
> 
> Why are these functions "protected"?   I don't see anything that inherits from FieldLayoutBuilder that might want to call these functions and not the other functions.

It’s a left over from a previous design.
Changed to private.

> 
> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/classfile/fieldLayoutBuilder.cpp.html
> 
> 544             _contended_groups = new (ResourceObj::RESOURCE_AREA, mtInternal) GrowableArray<FieldGroup*>(8);
> 
> 
> Growable arrays are default ResourceObj, so I think the 'new' arguments aren't needed (there were a couple of these).

I’ve changed _contended_groups’ type to GrowableArray<FieldGroup*> instead of GrowableArray<FieldGroup*>*
It removes the “new” and simplifies the management of the _contended_groups field.

> 
> 698   OopMapBlocksBuilder* nonstatic_oop_maps =
> 699       new OopMapBlocksBuilder(max_oop_map_count, Thread::current());
> 
> The code uses OopMapBlocksBuilder as a StackObj, which seems to make more sense.  Can you make it StackObj and not have it allocate from resourceArea?

nonstatic_oop_maps is not used as StackObj, but as a ResourceObj, it escapes the method at the end,
when the FieldLayoutInfo is filled.

> 
> 531 void FieldLayoutBuilder::regular_field_sorting(TRAPS) {
> 
> 635   regular_field_sorting(CHECK);
> 
> None of these functions throw Java class exceptions.  They shouldn't pass TRAPS and CHECK which are for Java exceptions.  If you want to avoid JavaThread::current, you can pass JavaThread* thread as the first parameter.

Fixed

>  I think for this change, it might be better to let this:
> 
> + OopMapBlock* oop_maps_copy = NEW_RESOURCE_ARRAY_IN_THREAD(THREAD, OopMapBlock,
> 
> be
> 
> + OopMapBlock* oop_maps_copy = NEW_RESOURCE_ARRAY(OopMapBlock,
> 
> 
> and get it's own Thread::current().  I don't think it saves many instructions and definitely not any time.

Fixed

> 
> +void OopMapBlocksBuilder::compact(TRAPS) {
> 
> 
> This shouldn't take a thread argument, so shouldn't be declared with TRAPS, see above.

Fixed

> 
> 748   if (PrintFieldLayout) {
> 
> 
> In some future change, this should be turned into unified logging.

Regarding the complexity of the task (considering the amount of data that can be generated
and the variations in the output format), I’d prefer to address this conversion in a separate
changeset.

> 
> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/src/hotspot/share/classfile/classFileParser.cpp.udiff.html
> 
> Why was AnnotationCollector moved to the header file?  It seems only used by classFileParser.cpp?

I needed it here during some steps of the development, but this move is not required anymore,
I moved the class back to the .cpp file.

> 
> This is a really nice change and your comments are really good. These are all pretty small comments.
> 

Thank you for this in depth review.

Fred

> Thanks,
> Coleen
> 
> On 1/23/20 10:03 AM, Frederic Parain wrote:
>> Greetings,
>> 
>> Please review this change proposing a new code to compute field layouts.
>> 
>> CR: https://bugs.openjdk.java.net/browse/JDK-8237767
>> webrev: http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.04/
>> 
>> The CR includes a detailed description of the motivation and the implementation.
>> 
>> The current version keeps the old code accessible (with a VM flag) in case the
>> small changes in computed layouts cause troubles to some applications or tools.
>> The goal is to get rid of this old code, preferably as soon as possible.
>> 
>> Testing tier1-8.
>> 
>> Thank you,
>> 
>> Fred
>> 
> 