RFR[L]: 8237767 Field layout computation overhaul

[coleen.phillimore at oracle.com]

I meant to say that I have no further comments.  Looks good.
Coleen

On 2/6/20 5:59 PM, 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
>>
>