RFR[L]: 8237767 Field layout computation overhaul
David Holmes
david.holmes at oracle.com
Fri Jan 31 00:23:50 UTC 2020
Correction ...
On 31/01/2020 9:40 am, David Holmes wrote:
> On 31/01/2020 1:04 am, coleen.phillimore at oracle.com wrote:
>> On 1/29/20 6:44 PM, David Holmes wrote:
>>> 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.
>>
>> How about add flag for 15, pre-deprecated, obsolete in 16 so that
>> valhalla work can go forward, especially if there's a preview in 16.
>> Either way the old code is removed in JDK 17, the LTS release.
>
> That is fine too.
>
>>> 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.
>>
>> This 6 months of support vs. 9 months isn't going to make a practical
>> difference to the customers. They can fall back on JDK 11, the
>> previous LTS release until they fix their code. 6 months of "support"
>> is better for us however since we don't have code paths that we should
>> be testing, and will be a broken (or preferably disabled) code path in
>> the valhalla repo that we're working on and asking people to try right
>> now.
>>>
>>> If we intend to tell anyone to use the flag then we have to ensure
>>> both code paths remain fully functional during that period.
>>
>> That's the thing. We don't _want_ to tell anyone to use this flag
>> unless they are diagnosing a problem with their code.
>
> Regardless of the "why" if we tell people to use this flag as a
> workaround then the code path has to be functional.
>
>> The only reason I can see making it a product flag is because we want
>> customers to fix their code, and not us to add hacks to our new code
>> to support any corner case layout that _might_ exist.
>
> A product flag is good for users and has no downside for us. A
> diagnostic flag is not so good for users and has no upside for us. The
> old code is going to remain, and must continue to work, for the exact
> same length of time. Having one product flag and one diagnostic flag
> makes no sense to me.
Sorry I just realized that the two flags are unrelated. One pertains to
using the old code versus new; the other controls a more aggressive
optimisation in the new code.
David
> Cheers,
> David
>
>> Coleen
>>>
>>> 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