RFR[L]: 8237767 Field layout computation overhaul

[coleen.phillimore at oracle.com]

On 1/29/20 8:31 AM, David Holmes wrote:
> 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.

Obviously I disagree.  We don't want to publicise this flag *unless* 
someone has a problem, which makes it a diagnostic flag.   We don't want 
to have to carry the old code 2 releases when there is honestly no 
reason to suspect it is problematic.

Coleen

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