RFR[L]: 8237767 Field layout computation overhaul

[coleen.phillimore at oracle.com]

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

+ 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.

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.

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?

   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).

  132   static const int INITIAL_LIST_SIZE;


It's odd to see this without an initialization.

  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.

  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.

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).

  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?

  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.  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.

+void OopMapBlocksBuilder::compact(TRAPS) {


This shouldn't take a thread argument, so shouldn't be declared with 
TRAPS, see above.

  748   if (PrintFieldLayout) {


In some future change, this should be turned into unified logging.

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?

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

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
>