RFR[L]: 8237767 Field layout computation overhaul
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Thu Jan 30 15:04:43 UTC 2020
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.
>
> 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. 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.
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