8247357: Flattenable field concept needs some cleanup

[fparain]

Please review these changes cleaning up the flattenable field concept.
The concept has evolved with time and now all fields with an inline type are by definition flattenable, so the need to have a "flattenable bit" on the side is gone. The changeset contains a mix of renaming and code cleaning.
The changes don't include JIT code, which would be fix in a follow-up patch.

Thank you,

Fred

---

Progress

• Change must not contain extraneous whitespace

Issue

• JDK-8247357: Flattenable field concept needs some cleanup

Reviewers

• Harold Seigel (hseigel - Committer) ⚠️ Review applies to 5b49d2d

Download

$ git fetch https://git.openjdk.java.net/valhalla pull/77/head:pull/77
$ git checkout pull/77

[bridgekeeper]

👋 Welcome back fparain! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request.

[openjdk]

@fparain This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

8247357: Flattenable field concept needs some cleanup

Reviewed-by: hseigel

• If you would like to add a summary, use the /summary command.
• To credit additional contributors, use the /contributor command.
• To add additional solved issues, use the /issue command.

There are currently no new commits on the lworld branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate be9744da79fa9cc5c4af6285a76cbdeccfe7350a.

➡️ To integrate this PR with the above commit message to the lworld branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 03: Full - Incremental (3acfbe2)
• 02: Full (5b49d2d)
• 01: Full (f01e45d)
• 00: Full (55ac05e)

[openjdk]

@fparain this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout flattenable_squashed2
git fetch https://git.openjdk.java.net/valhalla lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

[mlbridge]

Mailing list message from John Rose on valhalla-dev:

On Jun 10, 2020, at 11:55 AM, Frederic Parain <fparain at openjdk.java.net> wrote:

Please review these changes cleaning up the flattenable field concept.
The concept has evolved with time and now all fields with an inline type are by definition flattenable, so the need to
have a "flattenable bit" on the side is gone. The changeset contains a mix of renaming and code cleaning. The changes
don't include JIT code, which would be fix in a follow-up patch.

I?m glad to see occurrences of ?value? go away in favor of ?inline?.

(The language gurus won?t absolutely promise that ?inline? is the
final word on terminology, but I think at least for the JVM code,
?inline? is a far more descriptive term than ?value?.)

The ?is_flattenable? bit was often accompanied by an ?is_flattened?
bit which reported whether the intended flattening actually took place.
Having their names be similar helped make the code understandable.
So I suggest renaming those guys also, since you are touching all
that code now.

The term ?is_inline? is ambiguous when reading the code. Where there
is any doubt about whether it means ?is intended to be inlined? vs.
?is actually inlined in the layout?, the term should be made more explicit.

So I suggest:

s/is_flattenable/is_declared_inline/
s/is_flattened/is_allocated_inline/

Maybe that?s overkill? But I think just ?is_inline? is not clear enough.

? John

[mlbridge]

Mailing list message from John Rose on valhalla-dev:

On Jun 10, 2020, at 12:30 PM, John Rose <john.r.rose at oracle.com> wrote:

So I suggest:

s/is_flattenable/is_declared_inline/
s/is_flattened/is_allocated_inline/

Maybe that?s overkill? But I think just ?is_inline? is not clear enough.

To be clear: I?m not suggesting that systematically, just where
the distinction exists between ?could be? and ?actually is? flattened.
Names like STATIC_INLINE in the CFP are completely unambiguous;
they shouldn?t be STATIC_ALLOCATED_INLINE or the like.

In the CFP, ?has_flattenable_fields? could go either way, but I think
?has_declared_inline_fields? would be safer. It?s not clear whether it
means ?my definer has declared inline fields in me?, or ?I actually
flattened one or more of my fields?.

[mlbridge]

Mailing list message from John Rose on valhalla-dev:

[consolidate and resend after name fix]

On Jun 10, 2020, at 11:55 AM, Frederic Parain <fparain at openjdk.java.net> wrote:

Please review these changes cleaning up the flattenable field concept.
The concept has evolved with time and now all fields with an inline type are by definition flattenable, so the need to
have a "flattenable bit" on the side is gone. The changeset contains a mix of renaming and code cleaning. The changes
don't include JIT code, which would be fix in a follow-up patch.

I?m glad to see occurrences of ?value? go away in favor of ?inline?.

(The language gurus won?t absolutely promise that ?inline? is the
final word on terminology, but I think at least for the JVM code,
?inline? is a far more descriptive term than ?value?.)

The ?is_flattenable? bit was often accompanied by an ?is_flattened?
bit which reported whether the intended flattening actually took place.
Having their names be similar helped make the code understandable.
So I suggest renaming those guys also, since you are touching all
that code now.

The term ?is_inline? is ambiguous when reading the code. Where there
is any doubt about whether it means ?is intended to be inlined? vs.
?is actually inlined in the layout?, the term should be made more explicit.

So I suggest:

s/is_flattenable/is_declared_inline/
s/is_flattened/is_allocated_inline/

Maybe that?s overkill? But I think just ?is_inline? is not clear enough.

? John

P.S. To be clear: I?m not suggesting that systematically, just where
the distinction exists between ?could be? and ?actually is? flattened.
Names like STATIC_INLINE in the CFP are completely unambiguous;
they shouldn?t be STATIC_ALLOCATED_INLINE or the like.

In the CFP, ?has_flattenable_fields? could go either way, but I think
?has_declared_inline_fields? would be safer. It?s not clear whether it
means ?my definer has declared inline fields in me?, or ?I actually
flattened one or more of my fields?.

[mlbridge]

Mailing list message from Frederic Parain on valhalla-dev:

John,

Thank you for looking at these changes.

?is_inline? might be confusing in the sense that it can be interpreted
as a property of the field layout. And ?is_declared_inline? shares the
same issue (could be interpreter as a field modifier).
What the is_inline() methods really do, is to answer the question:
is the type of this field an inline type? So, it?s a type question,
and not a layout question. And sometimes, we use is_inline to perform
checks that are not related to the layout, but to the properties of
the type (like null-freeness).

To prevent the confusion, I would propose to change ?is_inline? to
?is_inline_type?, so the it would be obvious that the test is about
the type of the field.

And to have similar names, we would follow your suggestion and
rename ?is_flattened? to ?is_allocated_inlined"

So:
if(fd->is_inline_type()) { // -> clearly a type test

and
if(fd->is_allocated_inline()) { // -> clearly a layout test

Would these new names address the concerns you have?

Regards,

Fred

On Jun 10, 2020, at 15:37, John Rose <john.r.rose at oracle.com> wrote:

[consolidate and resend after name fix]

On Jun 10, 2020, at 11:55 AM, Frederic Parain <fparain at openjdk.java.net> wrote:

Please review these changes cleaning up the flattenable field concept.
The concept has evolved with time and now all fields with an inline type are by definition flattenable, so the need to
have a "flattenable bit" on the side is gone. The changeset contains a mix of renaming and code cleaning. The changes
don't include JIT code, which would be fix in a follow-up patch.

I?m glad to see occurrences of ?value? go away in favor of ?inline?.

(The language gurus won?t absolutely promise that ?inline? is the
final word on terminology, but I think at least for the JVM code,
?inline? is a far more descriptive term than ?value?.)

The ?is_flattenable? bit was often accompanied by an ?is_flattened?
bit which reported whether the intended flattening actually took place.
Having their names be similar helped make the code understandable.
So I suggest renaming those guys also, since you are touching all
that code now.

The term ?is_inline? is ambiguous when reading the code. Where there
is any doubt about whether it means ?is intended to be inlined? vs.
?is actually inlined in the layout?, the term should be made more explicit.

So I suggest:

s/is_flattenable/is_declared_inline/
s/is_flattened/is_allocated_inline/

Maybe that?s overkill? But I think just ?is_inline? is not clear enough.

? John

P.S. To be clear: I?m not suggesting that systematically, just where
the distinction exists between ?could be? and ?actually is? flattened.
Names like STATIC_INLINE in the CFP are completely unambiguous;
they shouldn?t be STATIC_ALLOCATED_INLINE or the like.

In the CFP, ?has_flattenable_fields? could go either way, but I think
?has_declared_inline_fields? would be safer. It?s not clear whether it
means ?my definer has declared inline fields in me?, or ?I actually
flattened one or more of my fields?.

[mlbridge]

Mailing list message from John Rose on valhalla-dev:

On Jun 10, 2020, at 1:12 PM, Frederic Parain <frederic.parain at oracle.com> wrote:

John,

Thank you for looking at these changes.

?is_inline? might be confusing in the sense that it can be interpreted
as a property of the field layout. And ?is_declared_inline? shares the
same issue (could be interpreter as a field modifier).
What the is_inline() methods really do, is to answer the question:
is the type of this field an inline type? So, it?s a type question,
and not a layout question. And sometimes, we use is_inline to perform
checks that are not related to the layout, but to the properties of
the type (like null-freeness).

To prevent the confusion, I would propose to change ?is_inline? to
?is_inline_type?, so the it would be obvious that the test is about
the type of the field.

And to have similar names, we would follow your suggestion and
rename ?is_flattened? to ?is_allocated_inlined"

So:
if(fd->is_inline_type()) { // -> clearly a type test

and
if(fd->is_allocated_inline()) { // -> clearly a layout test

Would these new names address the concerns you have?

Yes, that?s great. Tiny tweak: I suggest ?is_allocated_inline?,
or ?is_inline_allocated? since the word ?inline? can function
as an adverb. (I?m not sure, but I think you are suggesting
?inlined? for ?inline?.)

[mlbridge]

Mailing list message from Frederic Parain on valhalla-dev:

Sure, ?is_allocated_inline? (the ?d? at the end was a typo,
in the code example, it is written correctly).

Thank you,

Fred

On Jun 10, 2020, at 16:23, John Rose <john.r.rose at oracle.com> wrote:

On Jun 10, 2020, at 1:12 PM, Frederic Parain <frederic.parain at oracle.com> wrote:

John,

Thank you for looking at these changes.

?is_inline? might be confusing in the sense that it can be interpreted
as a property of the field layout. And ?is_declared_inline? shares the
same issue (could be interpreter as a field modifier).
What the is_inline() methods really do, is to answer the question:
is the type of this field an inline type? So, it?s a type question,
and not a layout question. And sometimes, we use is_inline to perform
checks that are not related to the layout, but to the properties of
the type (like null-freeness).

To prevent the confusion, I would propose to change ?is_inline? to
?is_inline_type?, so the it would be obvious that the test is about
the type of the field.

And to have similar names, we would follow your suggestion and
rename ?is_flattened? to ?is_allocated_inlined"

So:
if(fd->is_inline_type()) { // -> clearly a type test

and
if(fd->is_allocated_inline()) { // -> clearly a layout test

Would these new names address the concerns you have?

Yes, that?s great. Tiny tweak: I suggest ?is_allocated_inline?,
or ?is_inline_allocated? since the word ?inline? can function
as an adverb. (I?m not sure, but I think you are suggesting
?inlined? for ?inline?.)

[mlbridge]

Mailing list message from Tobias Hartmann on valhalla-dev:

Just wondering if "is_inlined()" wouldn't be an option as well? "Allocated" sounds like there is
some heap allocation for that field going on.

Best regards,
Tobias

On 10.06.20 22:35, Frederic Parain wrote:

Sure, ?is_allocated_inline? (the ?d? at the end was a typo,
in the code example, it is written correctly).

Thank you,

Fred

On Jun 10, 2020, at 16:23, John Rose <john.r.rose at oracle.com> wrote:

On Jun 10, 2020, at 1:12 PM, Frederic Parain <frederic.parain at oracle.com> wrote:

John,

Thank you for looking at these changes.

?is_inline? might be confusing in the sense that it can be interpreted
as a property of the field layout. And ?is_declared_inline? shares the
same issue (could be interpreter as a field modifier).
What the is_inline() methods really do, is to answer the question:
is the type of this field an inline type? So, it?s a type question,
and not a layout question. And sometimes, we use is_inline to perform
checks that are not related to the layout, but to the properties of
the type (like null-freeness).

To prevent the confusion, I would propose to change ?is_inline? to
?is_inline_type?, so the it would be obvious that the test is about
the type of the field.

And to have similar names, we would follow your suggestion and
rename ?is_flattened? to ?is_allocated_inlined"

So:
if(fd->is_inline_type()) { // -> clearly a type test

and
if(fd->is_allocated_inline()) { // -> clearly a layout test

Would these new names address the concerns you have?

Yes, that?s great. Tiny tweak: I suggest ?is_allocated_inline?,
or ?is_inline_allocated? since the word ?inline? can function
as an adverb. (I?m not sure, but I think you are suggesting
?inlined? for ?inline?.)

[MrSimms]

+1

Just wondering if "is_inlined()" wouldn't be an option as well? "Allocated" sounds like there is
some heap allocation for that field going on.

Best regards,
Tobias

[fparain]

OK, after another round of renaming:

• "is_inline_type" is used to test if the type of a field is an inline type or not
• "is_inlined" is used to test if the field has been flattened in the layout of its container

Both expression are close, and can easily be found together with grep.

I've tried to fix all the comments to align them with the new names of methods and fields.

The term "flattened" is still used for arrays.

[hseigel]

Hi Fred,
The changes look good. One minor misspelling in templateTable_x86.cpp, "fiel" instead of "field".
Also, should JVM_ACC_FIELD_INLINED be 4000 instead of 8000?
Thanks, Harold

[fparain]

Harold,

Thank you for reviewing this. I've fixed the typo and changed back the value of JVM_ACC_FIELD_INLINED (it doesn't make a difference right now, but there's no reason to change it).

Fred

[fparain]

/integrate

[openjdk]

@fparain
Pushed as commit 4d78c63.