RFR[L]: 8237767 Field layout computation overhaul

[coleen.phillimore at oracle.com]

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
>>>