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