6312651: Compiler should only use verified interface types for optimization #10901

rwestrel · 2022-10-28T12:29:15Z

This change is mostly the same I sent for review 3 years ago but was
never integrated:

https://mail.openjdk.org/pipermail/hotspot-compiler-dev/2019-May/033803.html

The main difference is that, in the meantime, I submitted a couple of
refactoring changes extracted from the 2019 patch:

8266550: C2: mirror TypeOopPtr/TypeInstPtr/TypeAryPtr with TypeKlassPtr/TypeInstKlassPtr/TypeAryKlassPtr
8275201: C2: hide klass() accessor from TypeOopPtr and typeKlassPtr subclasses

As a result, the current patch is much smaller (but still not small).

The implementation is otherwise largely the same as in the 2019
patch. I tried to remove some of the code duplication between the
TypeOopPtr and TypeKlassPtr hierarchies by having some of the logic
shared in template methods. In the 2019 patch, interfaces were trusted
when types were constructed and I had added code to drop interfaces
from a type where they couldn't be trusted. This new patch proceeds
the other way around: interfaces are not trusted when a type is
constructed and code that uses the type must explicitly request that
they are included (this was suggested as an improvement by Vladimir
Ivanov I think).

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-6312651: Compiler should only use verified interface types for optimization

Reviewers

Vladimir Ivanov (@iwanowww - Reviewer) ⚠️ Review applies to f49a042a
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10901

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

Using diff file

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

bridgekeeper · 2022-10-28T12:30:13Z

👋 Welcome back roland! 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 · 2022-10-28T12:32:53Z

@rwestrel The following label will be automatically applied to this pull request:

hotspot-compiler

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

mlbridge · 2022-10-28T12:42:28Z

Webrevs

iwanowww

Thanks, Roland! Overall, looks very good.

Submitted hs-tier1 - hs-tier4 testing. (Earlier, it went through hs-tier1 - hs-tier8 without new failures.)

Some minor comments/suggestions follow.

iwanowww · 2022-10-28T17:01:06Z

src/hotspot/share/ci/ciInstanceKlass.cpp

+GrowableArray<ciInstanceKlass*>* ciInstanceKlass::transitive_interfaces() const {
+  GrowableArray<ciInstanceKlass*>* result = NULL;
+  GUARDED_VM_ENTRY(
+    InstanceKlass* ik = get_instanceKlass();


Does it make sense to cache the result on ciInstanceKlass instance?

Maybe but it's not as simple as it seems (for the reason discussed below for vm classes). Some classes are allocated in a special arena because they are shared between compilations. A _transitive_interfaces array would need to be allocated in that same arena (otherwise if lazily allocated during a particular compilation it would be in the thread's resource are and its content would be wiped out when the compilation is over but still reachable). That's not straightforward with the current implementation as the arena a ciInstanceKlass is allocated in is not available to the ciInstanceKlass so that would require extra changes and I'm not sure the extra complexity is worth it.

scratch that. I see it's done for _nonstatic_fields and will do the same.

iwanowww · 2022-10-28T17:02:44Z

src/hotspot/share/ci/ciObjectFactory.cpp

+  if (vmClasses::name##_is_loaded()) {                               \
+    InstanceKlass* ik = vmClasses::name();                           \
+    ciEnv::_##name = get_metadata(ik)->as_instance_klass();          \
+    Array<InstanceKlass*>* interfaces = ik->transitive_interfaces(); \


What's the purpose of interface-related part of the code?

ciInstanceKlass objects for the vm classes all need to be allocated from the same long lived arena that's created in ciObjectFactory::initialize() because they are shared between compilations. Without that code, the ciInstanceKlass for a particular vm class is in the long lived arena but the ciInstanceKlass objects for the interfaces are created later when they are needed in the arena of some compilation. Once that compilation is over the interface objects are destroyed but still referenced from shared types such as TypeInstPtr::MIRROR.

Thanks, got it now. Is it really needed considering there's transitive_interfaces() call later?

iwanowww · 2022-10-28T17:10:45Z

src/hotspot/share/opto/type.hpp

  // If the klass is final, the resulting type will be exact.
-  static const TypeOopPtr* make_from_klass(ciKlass* klass) {
-    return make_from_klass_common(klass, true, false);
+  static const TypeOopPtr* make_from_klass(ciKlass* klass, bool trust_interface = false) {


I'd suggest to use an enum (trust_interfaces/ignore_interfaces) instead of a bool, so the intention is clear at call sites.

Good suggestion. I will make that change.

iwanowww · 2022-10-28T17:13:13Z

src/hotspot/share/opto/type.cpp

  mreg2type[Op_RegFlags] = TypeInt::CC;

+  TypeAryPtr::_array_interfaces = new TypePtr::InterfaceSet();
+  GrowableArray<ciInstanceKlass*>* array_interfaces = ciArrayKlass::interfaces();


Maybe move the code into a constructor or a factory method?
After that, the only user of TypePtr::InterfaceSet::add() will be TypePtr::interfaces().
It would be nice to make TypePtr::InterfaceSet immutable and cache query results (InterfaceSet::is_loaded() and InterfaceSet::exact_klass()).

Good suggestion as well.

iwanowww · 2022-10-29T00:04:33Z

src/hotspot/share/opto/type.cpp

      }
-      return TypeInstPtr::make(ptr, ciEnv::current()->Object_klass(), false, NULL, offset, instance_id, speculative, depth);
+      interfaces = this_interfaces.intersection_with(tp_interfaces);
+      return TypeInstPtr::make(ptr, ciEnv::current()->Object_klass(), interfaces, false, NULL,offset, instance_id, speculative, depth);


NULL,offset

missing space

iwanowww · 2022-10-29T00:07:30Z

src/hotspot/share/opto/type.cpp

      // below the centerline when the superclass is exact. We need to
      // do the same here.
-      if (klass()->equals(ciEnv::current()->Object_klass()) && !klass_is_exact()) {
+      if (klass()->equals(ciEnv::current()->Object_klass()) && this_interfaces.intersection_with(tp_interfaces).eq(this_interfaces) && !klass_is_exact()) {


this_interfaces.intersection_with(tp_interfaces).eq(this_interfaces)

Maybe a case for a helper method InterfaceSet::contains(InterfaceSet)?

Indeed, there are several uses of this pattern. I will make that change.

iwanowww · 2022-10-29T00:10:00Z

src/hotspot/share/opto/type.cpp

+    ciInstanceKlass* ik = k->as_instance_klass();
+    bool klass_is_exact = ik->is_final();
+    if (!klass_is_exact &&
+        deps != NULL && UseUniqueSubclasses) {


Please, put UseUniqueSubclasses guard at the top of the method.

iwanowww · 2022-11-07T23:51:44Z

BTW will there still be a need in CastPP vs CheckCastPP dichotomy once the patch goes in?

rwestrel · 2022-11-08T15:30:52Z

BTW will there still be a need in CastPP vs CheckCastPP dichotomy once the patch goes in?

In principle, probably not.

rwestrel · 2022-11-09T14:43:25Z

Thanks, Roland! Overall, looks very good.

Thanks for reviewing this. I pushed new commits that should address your comments/suggestions.

iwanowww

Much better, thanks!

Minor comments/suggestions follow.

FTR test results are clean. I'll submit performance testing shortly.

iwanowww · 2022-11-09T18:32:12Z

src/hotspot/share/ci/ciObjectFactory.cpp

+  if (vmClasses::name##_is_loaded()) {                               \
+    InstanceKlass* ik = vmClasses::name();                           \
+    ciEnv::_##name = get_metadata(ik)->as_instance_klass();          \
+    Array<InstanceKlass*>* interfaces = ik->transitive_interfaces(); \


Thanks, got it now. Is it really needed considering there's transitive_interfaces() call later?

iwanowww · 2022-11-10T00:06:40Z

src/hotspot/share/ci/ciArrayKlass.cpp

  }
 }
+
+GrowableArray<ciInstanceKlass*>* ciArrayKlass::interfaces() {


FTR there's a subtle asymmetry between ciArrayKlass::interfaces() and ciInstanceKlass::transitive_interfaces() when it comes to memory allocation: the former allocates from resource area while the latter from compiler arena.

It doesn't cause any problems since ciArrayKlass::interfaces() is used only in Type::Initialize_shared() to instantiate shared TypeAryPtr::_array_interfaces, but it took me some time to find that out.

Maybe a helper method in type.cpp is a better place for that logic.

How would that work? ciArrayKlass::interfaces() has to transition into the vm which is not something that would feel right in type.cpp.

Indeed, good point.

IMO you could just work directly with CI mirrors for Serializable and Cloneable, but now I'm curious why does CI code diverge from ObjArrayKlass::compute_secondary_supers()?
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/oops/objArrayKlass.cpp#L376

And to answer my question: only Serializable and Cloneable are interfaces among those. The rest are arrays.

iwanowww · 2022-11-10T00:09:38Z

src/hotspot/share/ci/ciInstanceKlass.cpp

+GrowableArray<ciInstanceKlass*>* ciInstanceKlass::transitive_interfaces() {
+  if (_transitive_interfaces == NULL) {
+    GUARDED_VM_ENTRY(
+            InstanceKlass* ik = get_instanceKlass();


A candidate for compute_transitive_interfaces() helper method?

Done in new commit.

iwanowww · 2022-11-10T00:10:25Z

src/hotspot/share/opto/subnode.cpp

  // return the ConP(Foo.klass)
  assert(mirror_type->is_klass(), "mirror_type should represent a Klass*");
-  return phase->makecon(TypeKlassPtr::make(mirror_type->as_klass()));
+  return phase->makecon(TypeKlassPtr::make(mirror_type->as_klass(),  Type::trust_interfaces));


Extra space.

iwanowww · 2022-11-10T00:11:52Z

src/hotspot/share/opto/type.cpp

+    //
+    assert(loaded->ptr() != TypePtr::Null, "insanity check");
+    //
+    if(      loaded->ptr() == TypePtr::TopPTR ) { return unloaded; }


Missing space after if.

iwanowww · 2022-11-10T00:11:55Z

src/hotspot/share/opto/type.cpp

+
+  // Both are unloaded, not the same class, not Object
+  // Or meet unloaded with a different loaded class, not java/lang/Object
+  if( ptr != TypePtr::BotPTR ) {


Missing space after if.

rwestrel · 2022-11-10T09:57:34Z

Much better, thanks!

Minor comments/suggestions follow.

FTR test results are clean. I'll submit performance testing shortly.

Thanks. I updated the patch and removed the interface specific code in the VM_CLASS_DEFN() macro that's indeed no longer needed.

iwanowww · 2022-11-11T00:00:08Z

FTR performance results look good.

iwanowww

Great work, Roland! I'm approving the PR. (hs-tier1 - hs-tier2 sanity testing passed with latest version.)

Feel free to handle ciArrayKlass::interfaces() as you find most appropriate.

openjdk · 2022-11-11T01:05:51Z

@rwestrel 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:

6312651: Compiler should only use verified interface types for optimization

Reviewed-by: vlivanov, kvn

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 167 new commits pushed to the master branch:

0ec575a: 8297289: problem list runtime/vthread/RedefineClass.java and TestObjectAllocationSampleEvent.java
dcb8375: 8245246: Deprecate -profile option in javac
52494df: 8290845: Consider an alternative item separator for multi-item option values
c56c69e: 8285604: closed sun/java2d/GdiRendering/ClipShapeRendering.java failed with "Incorrect color ffeeeeee instead of ff0000ff in pixel (100, 100)"
6fd1442: 8296743: Tighten Class.getModifiers spec for array classes
3a15e84: 8297258: Typo in java -help referencing -disable-@files instead of --disable-@files
43ce047: 8178698: javax/sound/midi/Sequencer/MetaCallback.java failed with timeout
035eaee: 8296324: JVMTI GetStackTrace truncates vthread stack trace for agents loaded into running VM
59a308b: 8296632: Write a test to verify the content change of TextArea sends TextEvent
11fc65f: 8023562: [TEST_BUG] java/awt/Mouse/EnterExitEvents/DragWindowTest.java failed on ubuntu 13 and mac 10.11
... and 157 more: https://git.openjdk.org/jdk/compare/d4376f8b55391485365797d1c4d0dbbc6ed2ad92...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.

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

rwestrel · 2022-11-15T08:55:00Z

Great work, Roland! I'm approving the PR. (hs-tier1 - hs-tier2 sanity testing passed with latest version.)

Feel free to handle ciArrayKlass::interfaces() as you find most appropriate.

Thanks for the review (and running tests).
I removed ciArrayKlass::interfaces() in the new commit. Is the resulting code what you suggested?

iwanowww · 2022-11-18T22:35:35Z

I removed ciArrayKlass::interfaces() in the new commit. Is the resulting code what you suggested?

Yes, it is much clearer now. Thanks.

vnkozlov

Nice work. Thanks!

rwestrel · 2022-11-21T08:46:43Z

@iwanowww @vnkozlov thanks for the reviews.

rwestrel · 2022-11-21T08:46:57Z

/integrate

openjdk · 2022-11-21T08:47:52Z

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

bcc6b12: 8296945: PublicMethodsTest is slow due to dependency verification with debug builds
fc61658: 8294591: Fix cast-function-type warning in TemplateTable
544e317: 8059632: Method reference compilation uses incorrect qualifying type
651e547: 8297217: Incorrect generation name in the heap verification log message with Serial GC
dd55310: 8297303: ProblemList java/awt/Mouse/EnterExitEvents/DragWindowTest.java on macosx-all
3ea8971: 8269817: serviceability/jvmti/DynamicCodeGenerated/DynamicCodeGeneratedTest.java timed out with -Xcomp
0a3b0fc: 8296784: Provide clean mallinfo/mallinfo2 wrapper for Linux glibc platforms
7b3d581: 8297293: Add java/nio/channels/FileChannel/FileExtensionAndMap.java to ProblemList
251e065: 8296764: NMT: reduce loads in os::malloc
0845b39: 8296796: Provide clean, platform-agnostic interface to C-heap trimming
... and 169 more: https://git.openjdk.org/jdk/compare/d4376f8b55391485365797d1c4d0dbbc6ed2ad92...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-21T08:48:13Z

@rwestrel Pushed as commit 45d1807.

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

interfaces

fe65125

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 28, 2022

whitespaces

2798601

openjdk bot added the rfr Pull request is ready for review label Oct 28, 2022

build fix

c892751

iwanowww reviewed Oct 29, 2022

View reviewed changes

rwestrel added 5 commits November 8, 2022 17:47

review

ac75a0c

review

9fd9ee0

review

d5b83fb

review

a055843

review

49d1bf3

iwanowww reviewed Nov 10, 2022

View reviewed changes

rwestrel added 2 commits November 10, 2022 09:07

Merge branch 'master' into JDK-6312651

8d7eaa9

review

f49a042

iwanowww approved these changes Nov 11, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 11, 2022

review

ff151ff

vnkozlov approved these changes Nov 19, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 21, 2022

openjdk bot closed this Nov 21, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 21, 2022

eme64 mentioned this pull request Oct 29, 2025

8370405: C2: mismatched store from MergeStores wrongly scalarized in allocation elimination #27997

Closed

3 tasks

6312651: Compiler should only use verified interface types for optimization #10901

6312651: Compiler should only use verified interface types for optimization #10901

Uh oh!

Conversation

rwestrel commented Oct 28, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 28, 2022

Uh oh!

openjdk bot commented Oct 28, 2022

Uh oh!

mlbridge bot commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

iwanowww left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwanowww commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwestrel commented Nov 8, 2022

Uh oh!

rwestrel commented Nov 9, 2022

Uh oh!

iwanowww left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwestrel commented Oct 28, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 28, 2022 •

edited

Loading

iwanowww commented Nov 7, 2022 •

edited

Loading

openjdk bot commented Nov 11, 2022 •

edited

Loading