RFR[L]: 8237767 Field layout computation overhaul

[Frederic Parain]

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.

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