RFR[L]: 8237767 Field layout computation overhaul
David Holmes
david.holmes at oracle.com
Thu Jan 30 23:40:58 UTC 2020
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.
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