8366024: Remove unnecessary InstanceKlass::cast()

[iklam]

We have a lot of InstanceKlass::cast(k) calls where k is statically known to be an InstanceKlass. I fixed many instances of this pattern:

InstanceKlass* x = ....;
Klass* s = x->super(); // should call java_super()
InstanceKlass::cast(s)->xyz();

The super() method has a very confusing API. It has the return type of Klass* because for for an ObjArrayKlass like [Ljava/lang/String;:

• super() returns [Ljava/lang/Object;
• java_super() returns Ljava/lang/Object;

However, for [Ljava/lang/Object;, all TypeArrayKlasses and all InstanceKlasses, super() and java_super() return an identical value of that always have the actual type of InstanceKlass*.

See here about the difference between super() and java_super():

jdk/src/hotspot/share/oops/klass.hpp

Lines 218 to 221 in 7b9969d

	// super() cannot be InstanceKlass* -- Java arrays are covariant, and _super is used
	// to implement that. NB: the _super of "[Ljava/lang/Integer;" is "[Ljava/lang/Number;"
	// If this is not what your code expects, you're probably looking for Klass::java_super().
	Klass* super() const { return _super; }

Unfortunately, we have a lot of code that incorrectly uses super() instead of java_super(), which leads to InstanceKlass::cast() calls. I tried to fixed a bunch of easy ones in this PR, although there are a few more to go.

I also fixed some calls to local_interafaces()->at() that widens the return type for InstanceKlass* to Klass*, which may lead to unnecessary InstanceKlass::cast() calls.

I also removed the Klass::superklass() API. This was used only in a few places and all of them can be safely replaced with Klass::java_super().

To avoid confusion, I think we should rename super() to something more obvious, but let's do that in a future PR.

---

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-8366024: Remove unnecessary InstanceKlass::cast() (Enhancement - P4)

Reviewers

• Coleen Phillimore (@coleenp - Reviewer)
• David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26908

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back iklam! 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]

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

8366024: Remove unnecessary InstanceKlass::cast()

Reviewed-by: coleenp, dholmes

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

• 80ab094: 8347707: Standardise the use of os::snprintf and os::snprintf_checked
• bdc3981: 8361521: BogusFocusableWindowState.java fails with StackOverflowError on Linux
• 9339a6a: 8361593: Commented dead code in JDK-8342868 can be removed
• ... and 92 more: https://git.openjdk.org/jdk/compare/c01b4fc348fff37c502d38ab3bb3385a5a8cff9a...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.

[openjdk]

@iklam The following labels will be automatically applied to this pull request:

• graal
• hotspot

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

• 01: Full - Incremental (074ea8b2)
• 00: Full (930d6524)

[rose00]

Not necessarily for this PR, but I nominate jvm_super as the "true name" for super.

[adinn]

I found two more occurrences of casting super() to InstanceKlass:

src/hotspot/share/jfr/leakprofiler/chains/edgeUtils.cpp:79

    ik = (const InstanceKlass*)ik->super();

src/hotspot/share/prims/jni.cpp:209

    Klass* field_klass = k;
    Klass* super_klass = field_klass->super();
    // With compressed oops the most super class with nonstatic fields would
    // be the owner of fields embedded in the header.
    while (InstanceKlass::cast(super_klass)->has_nonstatic_fields() &&
           InstanceKlass::cast(super_klass)->contains_field_offset(offset)) {
      field_klass = super_klass;   // super contains the field also
      super_klass = field_klass->super();
    }

The first one ought perhaps to be using InstanceKlass::superklass()?

[adinn]

What is wrong with using superklass() here? We already know that klass is an InstanceKlass* so we don't need to make a virtual call to java_super().

[iklam]

I got rid of superklass() because it was yet another way of looking for a "super" that has no documentation.

In this particular case, the cost of a virtual code will be negligible compared to what happens in the following loop.

[coleenp]

This looks good. We've had several passes of narrowing the Klass types to InstanceKlass so thank you for doing this one. super() is tricky because of array covariance in subtype checking. I don't see that this affects that code.
Thank @adinn for finding more.

[iklam]

I found two more occurrences of casting super() to InstanceKlass:

src/hotspot/share/jfr/leakprofiler/chains/edgeUtils.cpp:79

    ik = (const InstanceKlass*)ik->super();

I fixed this one with java_super(). Similar to the case in deoptimization.cpp, the cost is dominated by the JavaFieldStream so a virtual call should be fine here.

intptr_t jfieldIDWorkaround::encode_klass_hash(Klass* k, int offset) {
 if (offset <= small_offset_mask) {
    Klass* field_klass = k;
    Klass* super_klass = field_klass->super();
    // With compressed oops the most super class with nonstatic fields would
    // be the owner of fields embedded in the header.
    while (InstanceKlass::cast(super_klass)->has_nonstatic_fields() &&
           InstanceKlass::cast(super_klass)->contains_field_offset(offset)) {
      field_klass = super_klass;   // super contains the field also
      super_klass = field_klass->super();
    }

Here we have the same logic as in the (removed) superklass() function, without any explanation why the cast is safe. The code seems to assume that the incoming k parameter cannot be a type such as [Ljava/lang/String;. I am not familiar with this code, so I will leave it as is.

[dholmes-ora]

FWIW jfieldIDWorkaround::encode_klass_hash does seem to end up getting called only when k is an instanceKlass but we should file a cleanup RFE to change the parameter type.

[iklam]

FWIW jfieldIDWorkaround::encode_klass_hash does seem to end up getting called only when k is an instanceKlass but we should file a cleanup RFE to change the parameter type.

I have filed JDK-8366417 - jfieldIDWorkaround should use InstanceKlass* instead of Klass*

[dholmes-ora]

This seems fine though I will also comment that java_super seems completely mis-named in relation to super as there is nothing more Java about it. The implementation of java_super for arrays is quite odd - I'm not sure when we don't care about the actual superclass and want to go straight to object.

Thanks

[iklam]

Thanks @adinn @dholmes-ora @coleenp for the review
/integrate

[openjdk]

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

• 80ab094: 8347707: Standardise the use of os::snprintf and os::snprintf_checked
• bdc3981: 8361521: BogusFocusableWindowState.java fails with StackOverflowError on Linux
• 9339a6a: 8361593: Commented dead code in JDK-8342868 can be removed
• ... and 92 more: https://git.openjdk.org/jdk/compare/c01b4fc348fff37c502d38ab3bb3385a5a8cff9a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@iklam Pushed as commit 2427c90.

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

[iklam]

This seems fine though I will also comment that java_super seems completely mis-named in relation to super as there is nothing more Java about it. The implementation of java_super for arrays is quite odd - I'm not sure when we don't care about the actual superclass and want to go straight to object.

As we discussed offline, I will try to add a new InstanceKlass* InstanceKlass::super() method. Then in most case people can just call ik->super() and it will do "the right thing".