RFR[L]: 8237767 Field layout computation overhaul

[Frederic Parain]

Hi David,

I’ve fixed all the issues you mentioned below.

According to the discussion about the VM flags, the following modifications
have also been made:

globals.hpp:
  UseNewFieldLayout is now a deprecated product flag

fieldLayoutBuilder.cpp
  lines 350-362:
  	UseEmptySlotsInSupers VM option now controls both new optimizations
  lines 449-464
	Fix an issue with layout printing (inherited fields were printed
	only for the direct super-class, now all inherited fields are printed)

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


CR and CSR have been updated accordingly (new VM flags explanations in the CSR).

Thank you,

Fred


> On Jan 29, 2020, at 02:06, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Fred,
> 
> I've looked at the v7 version. A few more stylistic comments on that first. Note, no need for an item by item response unless that makes it easier for you to track :)
> 
> src/hotspot/share/classfile/classFileParser.cpp
> 
> #include "classfile/defaultMethods.hpp"
> +#include "classfile/fieldLayoutBuilder.hpp"
> #include "classfile/dictionary.hpp"
> 
> Include files are not in alphabetical order.
> 
> +   * This may well change: FixMe if doesn't,
> 
> s/if/if it/
> 
> +  //Make a temp copy, and iterate through and copy back into the orig
> 
> Space after //
> 
> s/orig/original/
> 
> +  OopMapBlock*  nonstatic_oop_map = _nonstatic_oop_maps;
> 
> Extra space after *
> 
> +  assert(ik->nonstatic_oop_map_count() == _field_info->oop_map_blocks->_nonstatic_oop_map_count,
> +     "sanity");
> 
> Second line needs to be indented further:
> 
>   assert(ik->nonstatic_oop_map_count() == _field_info->oop_map_blocks->_nonstatic_oop_map_count,
>          "sanity");
> 
> ---
> 
> src/hotspot/share/classfile/classFileParser.hpp
> 
> +public:
> +  OopMapBlock* _nonstatic_oop_maps;
> +  unsigned int _nonstatic_oop_map_count;
> +  unsigned int _max_nonstatic_oop_maps;
> +
> + public:
> 
> Second public uneeded. First public may be indented wrong (I'm not sure what the rule is - single space indent?)
> 
> class ClassFileParser {
> +  friend class FieldLayoutBuilder;
> +  friend class FieldLayout;
> 
>  class ClassAnnotationCollector;
>  class FieldAllocationCount;
>  class FieldAnnotationCollector;
> 
> Indents are different. I think the class forward declarations should have extra space.
> 
> ---
> 
> src/hotspot/share/oops/instanceKlass.hpp
> 
> +  void increment_count(int diff)     { _count += diff; }
> 
> Extra spaces before {
> 
> ---
> 
> src/hotspot/share/runtime/globals.hpp
> 
> +  diagnostic(bool, UseNewFieldLayout, true,     \
> +               "Use new algorithm to compute layouts")     \
> +     \
> +  product(bool, UseEmptySlotsInSupers, true,
> 
> Not sure I see why one flag is diagnostic and the other product. Do you expect people to need to disable using empty slots more so than needing to disable using the new field layout altogether?
> 
> ---
> 
> src/hotspot/share/classfile/fieldLayoutBuilder.cpp
> 
> +  assert(kind == EMPTY || kind == RESERVED || kind == PADDING || kind == INHERITED,
> +      "Otherwise, should use the constructor with a field index argument");
> 
> Indentation of second line is wrong.
> 
> +  assert(kind == REGULAR || kind == FLATTENED || kind == INHERITED,
> +      "Other kind do not have a field index");
> 
> Ditto.
> 
> 
> +  if (list == NULL) return;
> +  if (start == NULL) {
> +    start = this->_start;
> +  }
> 
> Inconsistent style for single statement if-blocks. Same thing later in the file.
> 
> +      output->print_cr(" @%d \"%s\" %s %d/%d %s",
> +          b->offset(),
> +          fi->name(_cp)->as_C_string(),
> +          fi->signature(_cp)->as_C_string(),
> +          b->size(),
> +          b->alignment(),
> +          "REGULAR");
> 
> Incorrect identation of continuing line. Same for all the following print blocks.
> 
> +  } else if (_classname == vmSymbols::java_lang_Boolean() ||
> +      _classname == vmSymbols::java_lang_Character() ||
> +      _classname == vmSymbols::java_lang_Float() ||
> +      _classname == vmSymbols::java_lang_Double() ||
> +      _classname == vmSymbols::java_lang_Byte() ||
> +      _classname == vmSymbols::java_lang_Short() ||
> +      _classname == vmSymbols::java_lang_Integer() ||
> +      _classname == vmSymbols::java_lang_Long()) {
> 
> Incorrect identation of continuing line.
> 
> ---
> 
> src/hotspot/share/classfile/fieldLayoutBuilder.hpp
> 
> +// and the boxing classes). The rational for having multiple methods
> 
> s/rational/rationale/
> 
> +  FieldLayoutBuilder(const Symbol* classname, const InstanceKlass* super_klass, ConstantPool* constant_pool,
> +      Array<u2>* fields, bool is_contended, FieldLayoutInfo* info);
> 
> Indentation wrong for continuing line.
> 
> +  int get_alignment() {
> +     assert(_alignment != -1, "Uninitialized");
> +     return _alignment;
> +   }
> 
> Indenting appears off by one.
> 
> ---
> 
> test/hotspot/jtreg/runtime/FieldLayout/FieldDensityTest.java
> 
> + * @run main/othervm -XX:+UseCompressedOops -XX:+UseCompressedClassPointers FieldDensityTest
> + * @run main/othervm -XX:+UseCompressedOops -XX:-UseCompressedClassPointers FieldDensityTest
> + * @run main/othervm -XX:-UseCompressedOops -XX:-UseCompressedClassPointers FieldDensityTest
> 
> The test won't run on 32-bit platforms as the compressed oops flags won't exist.
> 
> ---
> 
> Some follow up comments below ...
> 
> With trimming ...
> 
> On 25/01/2020 3:20 am, Frederic Parain wrote:
>>> On Jan 24, 2020, at 08:19, David Holmes <david.holmes at oracle.com> wrote:
>>> 
>>> 466     int super_flen   = super->nof_nonstatic_fields();
>>> 
>>> Could be folded directly into the assert so we don't call in product.
>> Calling not_nonstatic_fields() has the side effect to compute non-static fields,
>> which is required to get a correct value when reading super->_nonstatic_fields,
>> so the call is needed even in product builds.
> 
> Yuck! That's a horrible side-effect - but not your fault obviously. :) It would be better to have a nonstatic_fields() accessor that has the same lazy initialization side-effect.
> 
>>> General style issue: when breaking a long line with a method call, the new line (containing arguments) should be indented to the opening ( of the method call e.g.
> ...
>>> etc. This applies across all files.
>> Fixes applied lines 4003, 4011, 4041, 4138, 4143.
> 
> Fix was also needed in other files. Current issues highlighted above.
> 
>>> 
>>> src/hotspot/share/oops/instanceKlass.hpp
>>> 
>>> You need to be careful with _extra_flags usage if there can be concurrently updated bits. At the moment it looks like redefinition is a mutable dynamic property, whilst "contended annotations" should be a static immutable property - is that right?
>> Correct, _has_contended_annotations is a static immutable property, while _is_being_redefined is a mutable one.
> 
> Good to know. My concern is that if someone adds a new mutable flag bit the need for atomic updates may not be noticed. We got bitten by this in the past with a flag field and I think we eventually migrated all of the mutable bits out into their own field. (Coleen should recall that :) ).
> 
>>>  61     FLATTENED,     // flattened field
>>> 
>>> Does this have any meaning before inline types come in?
>> Yes, I wanted to reserved the entry in the enum.
> 
> Hmmm a tenuous "okay". Seems odd to require this now to support code that is still some way from joining mainline.
> 
>>> In FieldLayoutBuilder::epilogue you have a number of calls to Thread::current() as well as an implicit call when you use ResourceMarks. You should capture the current thread once in a local and reuse it.
>> Fixed
> 
> It seems that this fix is now not needed as there is only one use left of the new "thread" variable in the ResourceMark. So that can return to being:
> 
> ResourceMark rm;
> 
> Thanks,
> David
> -----
>