RFR[L]: 8237767 Field layout computation overhaul

[coleen.phillimore at oracle.com]

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
>