RFR[L]: 8237767 Field layout computation overhaul

[Frederic Parain]

The kind of the flag is a secondary issue, the main issue is “how
fast do we want to remove the old code?”. I wanted to keep the old
code for a short transition period in case new field layouts cause
issues for some tools or customers. The goal was not to keep it
forever and allow users to select which algorithm they want to use.

Note that keeping the old code and its activation with a normal
product flag will be an issue for the first release of Valhalla.
The way flattened fields are allocated in the old code is very
inefficient, which means that instead of seeing an improvement
in data density, users will see a significant increase in space
consumption if they use the old code.

So, here’s the three possible paths forward:
  1 - push the new code and remove the old code in a single change,
      so no need for a VM option to select which code to use
  2 - push the new code while keeping the old one for a short
      period of time, to remove the old code before Valhalla code
      is pushed
  3 - push the new code while keeping the old one with a long
      deprecation period before removing the old code

Option 3 will be an issue for project Valhalla, as explained
above.

Option 1 sounds a big risky to me, from the experience I had
while doing this change in the JVM and the unexpected places
where it caused issues.

Option 2 sounded like a good compromise, however, it seems
there’s no consensus how to implement it. Could we refine
the details on how a short transition period could be implemented?

Thank you,

Fred


> On Jan 29, 2020, at 08:44, coleen.phillimore at oracle.com wrote:
> 
> 
> 
> 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
>>> -----
>>> 
>