RFR[L]: 8237767 Field layout computation overhaul

[David Holmes]

PS. I've now seen the CSR and have commented there. I don't agree with 
the use of a diagnostic flag. It doesn't buy us anything other than not 
needing a CSR request when we decide to deprecate it. But if we're going 
to tell people to use these flags to revert behaviour then they will 
need to follow the deprecation process regardless - and a product flag 
is better from an end user perspective.

Cheers,
David

On 29/01/2020 5:06 pm, David Holmes 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
> -----
>