8348853: Fold layout helper check for objects implementing non-array interfaces

[marc-chevalier]

If TypeInstKlassPtr represents an array type, it has to be java.lang.Object. From contraposition, if it is not java.lang.Object, we can conclude it is not an array, and we can skip some array checks, for instance.

In this PR, we improve this deduction with an interface base reasoning: arrays implements only Cloneable and Serializable, so if a type implements anything else, it cannot be an array.

This change partially reverts the changes from JDK-8348631 (#23331) (in LibraryCallKit::generate_array_guard_common) and the test still passes.

The way interfaces are check might be done differently. The current situation is a balance between visibility (not to leak too much things explicitly private), having not overly general methods for one use-case and avoiding too concrete (and brittle) interfaces.

Tested with tier1..3, hs-precheckin-comp and hs-comp-stress

Thanks,
Marc

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8348853: Fold layout helper check for objects implementing non-array interfaces (Enhancement - P2)

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer)
• Roland Westrelin (@rwestrel - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24245/head:pull/24245
$ git checkout pull/24245

Update a local copy of the PR:
$ git checkout pull/24245
$ git pull https://git.openjdk.org/jdk.git pull/24245/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24245

View PR using the GUI difftool:
$ git pr show -t 24245

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24245.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mchevalier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@marc-chevalier This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8348853: Fold layout helper check for objects implementing non-array interfaces

Reviewed-by: thartmann, roland

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 28 new commits pushed to the master branch:

• f7fa05f: 8353698: Output of Simple Web Server is garbled if the console's encoding is not UTF-8
• 250eb74: 8353192: C2: Clean up x86 backend after 32-bit x86 removal
• 9ee5590: 8334046: Set different values for CompLevel_any and CompLevel_all
• ... and 25 more: https://git.openjdk.org/jdk/compare/d9f2e6921558b4919889d81871b699971fb4f3ba...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann, @rwestrel) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@marc-chevalier The following labels will be automatically applied to this pull request:

• graal
• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (b1fb82a2)
• 01: Full - Incremental (daaaf9ae)
• 00: Full (a77c397c)

[TobiHartmann]

@rwestrel Should have a look at this :)

Please add an IR framework test that verifies that layout helper checks are optimized.

[TobiHartmann]

What about using TypeAryPtr::_array_interfaces->contains(_interfaces); instead?

[marc-chevalier]

Almost!

return !TypeAryPtr::_array_interfaces->contains(this);

Contains is about TypeInterfaces, that is set of interfaces. So I just need to check that this is not a sub-set of array interfaces. That should do it.

[TobiHartmann]

Now I'm confused, isn't this what I proposed? I didn't check the exact syntax, I just wondered if the TypeInterfaces::contains method couldn't be used instead of adding a new method.

[marc-chevalier]

Yes, totally! It's just a detail difference. But there is another question: whether we still want has_non_array_interface has a wrapper for this call with a more explicit name, or if we simply inline your suggestion on the callsite of has_non_array_interface. I tend toward the first, I like explicit names, and I suspect it might be useful in more than one place, but not a strong opinion.

[marc-chevalier]

For now, I just replaced the implementation of has_non_array_interface. If one feels even keeping the method is premature factorization, I can easily inline it.

[marc-chevalier]

I'm not sure how to write such an IR test.

I'm looking at TestArrayGuardWithInterfaces.java.

I see the graphs of test1 before and after, and the new one is smaller. But the nodes used are pretty much the same, or they don't feel clearly linked to interface checking: there is DecodeNKlass or AddP, but it doesn't seem obvious without having the graph under the eyes that it actually checks something meaningful. There are also less If (2 instead of 3), but once again, the test seems brittle. I also see that There is no more Return only Halt since we can now prove the function cannot return normally.

But on the graph of test2 ends with two Halt: traps everywhere, even if there are paths on which test2 doesn't throw. So the lack of Return doesn't sound very robust.

Overall, not sure what a good test would be. I can write a test that would not pass before and pass now, but I'm not convinced they would reliably catch regression, and that they won't break for unrelated reasons.

[TobiHartmann]

Right, I was hoping that there would be some other suitable users of GraphKit::get_layout_helper that would now be folded but all current uses either trap or don't handle both arrays and non-arrays (and therefore wouldn't fold). So I agree, adding an IR framework test does not make sense. The existing test is sufficient.

[rwestrel]

Could we do the same by using TypeKlassPtr::maybe_java_subtype_of(TypeAryKlassPtr::BOTTOM) and define a TypeAryKlassPtr::BOTTOM to be a static field for the array_interfaces?

AFAICT, TypeKlassPtr::maybe_java_subtype_of() already covers that case so it would avoid some logic duplication. Also in the test above, maybe you could simplify the test a little but by removing tkls->isa_instklassptr()?

[marc-chevalier]

I think it should be

TypeAryKlassPtr::BOTTOM->maybe_java_subtype_of(tkls)

rather than

tkls->maybe_java_subtype_of(TypeAryKlassPtr::BOTTOM)

My reasoning: if TypeAryKlassPtr::BOTTOM is java.lang.Object + Cloneable + Serializable any array is a subtype of that. But so is any class implementing these interfaces. As well as as any Object implementing more interfaces. But for these two last cases, we know they cannot be array, which is what we want to know: are we sure it's not an array, or could it be an array?

But if we check if tkls is a supertype of java.lang.Object + Cloneable + Serializable, then it has to be an Object (the most general class) and it implements a subset of Cloneable and Serializable. In this case, it can be an array. If tkls is not a super-type of java.lang.Object + Cloneable + Serializable, there are 2 cases:

• either it is an array type directly (so, I think, in a way or another, we need to check for is_instklassptr), and so a fortiori it can be an array type.
• it's an instance type and then cannot be an array since there is nothing between array types and java.lang.Object + Cloneable + Serializable. I.e. there is no type T that is not an array type, that is a super-type of at least one array type and that is not a super-type of java.lang.Object + Cloneable + Serializable (that is that is not java.lang.Object or that implements at least another interface).

In other words, our question is

\exists T: T is an array type /\ T <= tkls

(where A <= B means A is a subtype of B) which is equivalent to

    tkls >= (java.lang.Object + Cloneable + Serializable)
\/ (tkls <= (java.lang.Object + Cloneable + Serializable) /\ tkls is an array type)

We can spare the call to is_instklassptr by using a virtual method instead or probably other mechanisms, that's an implementation detail. But I think we need to distinguish cases: both int[] and MyClass + Cloneable + Serializable + MyInterface are sub-types of java.lang.Object + Cloneable + Serializable but for one, we can conclude it's definitely an array, and the other, it's definitely not. Without distinguishing cases, the only sound approximation would be to that that everything can be an array (both sub and super types of java.lang.Object + Cloneable + Serializable).

Does that makes sense? Did I get something wrong? is the BOTTOM not what you had in mind?

[rwestrel]

Yes, what I suggested doesn't work indeed.

[TobiHartmann]

This looks good to me.

[rwestrel]

Looks good to me.

[marc-chevalier]

/integrate

The branch was a bit old, so I've merged master in it and run tests. It seems all good!
Thanks @TobiHartmann and @rwestrel for the reviews.

[openjdk]

@marc-chevalier
Your change (at version b1fb82a) is now ready to be sponsored by a Committer.

[TobiHartmann]

/sponsor

[openjdk]

Going to push as commit a1d566c.
Since your change was applied there have been 28 commits pushed to the master branch:

• f7fa05f: 8353698: Output of Simple Web Server is garbled if the console's encoding is not UTF-8
• 250eb74: 8353192: C2: Clean up x86 backend after 32-bit x86 removal
• 9ee5590: 8334046: Set different values for CompLevel_any and CompLevel_all
• ... and 25 more: https://git.openjdk.org/jdk/compare/d9f2e6921558b4919889d81871b699971fb4f3ba...master

Your commit was automatically rebased without conflicts.

[openjdk]

@TobiHartmann @marc-chevalier Pushed as commit a1d566c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.