RFR[L]: 8237767 Field layout computation overhaul

[Aleksey Shipilev]

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.

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?

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

*) "(t)he requirements"?

 179  // was successful. If he requirements were the same but the search failed, a new search will

*) "INHERITED bloc(k)s"?

 302  // INHERITED blocs are marked as non-reference because oop_maps are handled by their holder class

*) What is so suspicious about this test?

 389   if (block->prev_block() != NULL) {       // suspicious test

*) "!last_search_success" here?

  181     else  if (b->size() == last_size && b->alignment() == last_alignment &&
last_search_success == false) {

*) Still "first fit"?

 239 // The allocation logic uses a first fit strategy: the set of fields is allocated

*) 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).

*) 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;
    }

*) 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:
       ...
   }

-- 
Thanks,
-Aleksey