RFR[L]: 8237767 Field layout computation overhaul

[Frederic Parain]

Thank you Coleen.

Fred

> On Feb 7, 2020, at 08:13, coleen.phillimore at oracle.com wrote:
> 
> 
> 
> On 2/7/20 7:59 AM, Frederic Parain wrote:
>> Coleen,
>> 
>> I’ve rewritten the code as you suggested.
>> 
>> There’s no break because it is now a best-fit search,
>> the loop doesn’t stop when a first candidate is found,
>> but continues up to the start block to find the smallest
>> candidate.
> 
> Ok!  Sounds good.
> Coleen
> 
>> Thank you,
>> 
>> Fred
>> 
>> 
>>> On Feb 6, 2020, at 17:59, coleen.phillimore at oracle.com wrote:
>>> 
>>> 
>>> Except for the same comment as Aleksey about this:
>>> 
>>> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.09c-diff/src/hotspot/share/classfile/fieldLayoutBuilder.cpp.udiff.html
>>> 
>>> + while (cursor != start) {
>>> + if (cursor->kind() == LayoutRawBlock::EMPTY && cursor->fit(b->size(), b->alignment())) {
>>> + if (candidate == NULL) candidate = cursor;
>>> + else if (cursor->size() < candidate->size()) candidate = cursor;
>>> + }
>>> + cursor = cursor->prev_block();
>>> + }
>>> 
>>> Shouldn't it be:
>>> 
>>> + while (cursor != start) {
>>> + if (cursor->kind() == LayoutRawBlock::EMPTY && cursor->fit(b->size(), b->alignment())) {
>>> + if (candidate == NULL || cursor->size() < candidate->size()) { + candidate = cursor;
>>> +          }
>>> + }
>>> + cursor = cursor->prev_block();
>>> + }
>>> 
>>> 
>>> and should there be a 'break' once you've got your candidate?
>>> 
>>> Thanks,
>>> Coleen
>>> 
>>> On 2/6/20 1:54 PM, Frederic Parain wrote:
>>>> Aleksey,
>>>> 
>>>> Thank you the review, I’ve fixed all issues (except deprecating the
>>>> option, see below), and updated the webrev in place:
>>>> 
>>>> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.09c/index.html
>>>> 
>>>> 
>>>>> On Feb 6, 2020, at 13:05, Aleksey Shipilev <shade at redhat.com> wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> I have to say that seeing the new field layouter matching all the cases with perfect layouter
>>>>> simulator I did back in 2013 is very impressive! Good job.
>>>> Thank you. Your simulator helped finding some bugs and inefficiencies.
>>>> 
>>>>> On 2/6/20 5:36 PM, Frederic Parain wrote:
>>>>>> Here’s a new version of the code addressing these regressions:
>>>>>> http://cr.openjdk.java.net/~fparain/jdk_layout/webrev.09c/index.html
>>>>> Only the minor ones. I don't need another webrev, you might consider fixing them before push.
>>>>> 
>>>>> *) UseEmptySlotsInSupers should also be deprecated right away?
>>>> The plan was to keep this option on the long term, for use cases
>>>> where keeping fields from a given class close to each other is more
>>>> important than having a smaller instance size.
>>>> 
>>>>> === fieldLayoutBuilder.cpp:
>>>>> 
>>>>> *) Generally, it feels some method bodies would enjoy some new lines, to split logical parts of the
>>>>> method. For example, fillHoles() seems to break into five logical parts:
>>>>> https://paste.centos.org/view/1b7805f6
>>>> I’e added more new lines across this file to make the code easier to read.
>>>> 
>>>>> *) "(t)he requirements"?
>>>>> 
>>>>> 179  // was successful. If he requirements were the same but the search failed, a new search will
>>>> Fixed
>>>> 
>>>>> *) "INHERITED bloc(k)s"?
>>>>> 
>>>>> 302  // INHERITED blocs are marked as non-reference because oop_maps are handled by their holder class
>>>> Fixed
>>>> 
>>>>> *) What is so suspicious about this test?
>>>>> 
>>>>> 389   if (block->prev_block() != NULL) {       // suspicious test
>>>> After careful review, this case is not suspicious, it matches
>>>> one case of insertion. Comment removed.
>>>> 
>>>>> *) "!last_search_success" here?
>>>>> 
>>>>>  181     else  if (b->size() == last_size && b->alignment() == last_alignment &&
>>>>> last_search_success == false) {
>>>> Fixed.
>>>> 
>>>> 
>>>>> *) Still "first fit"?
>>>>> 
>>>>> 239 // The allocation logic uses a first fit strategy: the set of fields is allocated
>>>> Yes, FieldLayout::add_contiguously() still uses a first-fit strategy, but it is only
>>>> to allocate static fields.
>>>> 
>>>> However, I’ve update the comment before FieldLayout::add() to clarify the best-fit search.
>>>> 
>>>>> *) Still true? oop fields can now fill the gaps, no?
>>>>> 
>>>>> 598 //   - then oop fields are allocated contiguously (to reduce the number of oopmaps
>>>>> 599 //     and reduce the work of the GC).
>>>> Updated.
>>>> 
>>>>> *) Let's write this:
>>>>> 
>>>>> 192           if (candidate == NULL) candidate = cursor;
>>>>> 193           else if (cursor->size() < candidate->size()) candidate = cursor;
>>>>> 
>>>>> like this:
>>>>> 
>>>>>    if (candidate == NULL) {
>>>>>       candidate = cursor;
>>>>>    } else if (cursor->size() < candidate->size()) {
>>>>>       candidate = cursor;
>>>>>    }
>>>>> 
>>>> Definitively easier to read.
>>>> 
>>>>> *) I believe indenting switch cases is good style here:
>>>>> 
>>>>> 559     switch(type) {
>>>>> ...
>>>>> 570     case T_OBJECT:
>>>>> 571     case T_ARRAY:
>>>>> 572       if (group != _static_fields) _nonstatic_oopmap_count++;
>>>>> 573       group->add_oop_field(fs);
>>>>> ...
>>>>> 
>>>>> I.e.:
>>>>> 
>>>>>   switch(type) {
>>>>>     case T_OBJECT:
>>>>>     case T_ARRAY:
>>>>>       ...
>>>>>     default:
>>>>>       ...
>>>>>   }
>>>>> 
>>>> I’ve fixed indentation of several switch statements in this file.
>>>> 
>>>>> -- 
>>>>> Thanks,
>>>>> -Aleksey
>>>>> 
>>>> Thank you,
>>>> 
>>>> Fred
>>>> 
>