RFR[L]: 8237767 Field layout computation overhaul
David Holmes
david.holmes at oracle.com
Wed Jan 29 23:44:58 UTC 2020
Hi Fred,
On 30/01/2020 12:57 am, Frederic Parain wrote:
> 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.
Right. The problem with our 6 month release cadence is that we're not
realistically going to get this new code exposed to users who will find
potential problems in a single release. Even if we think this might
mainly impact tools and those developers do tend to try and keep up, it
is still unlikely to get enough visibility in a single release so ...
> 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
Seems potentially dangerous when we don't know the impact.
> 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
Begs the question: when do we think something of Valhalla will ship? I'm
guessing not before 17. If so then we deprecate the flag in 16 and
obsolete in 17 - thus zero impact on Valhalla. If Valhalla comes earlier
in 16 then we have some impact but will still have this out of the way
before 17 (which should be next LTS release).
> 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?
Add flag for 15; deprecate in 16; obsolete in 17; expire in 18.
I'm concerned we are developing a tendency to make flags diagnostic
because it is considered convenient for us. There are customers whose
own policies disallow running their production systems with anything
other than product flags. To me a diagnostic flag aids in diagnosing a
problem more than being a switch between old and new behaviour - but I
agree the lines are blurry. Historically there was much more difference
between product flags and diagnostic, when product flags would be
supported in a number of releases spanning many years. That is no longer
the case. We can effectively kill off a product flag after 12 months: 6
months full "support"; 6 months deprecated; then obsolete. We can even
deprecate a flag in the release we introduce it if we really think that
is warranted.
If we intend to tell anyone to use the flag then we have to ensure both
code paths remain fully functional during that period.
Cheers,
David
> 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
>>>> -----
>>>>
>>
>
More information about the hotspot-dev
mailing list