Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8264732: Clean up LinkResolver::vtable_index_of_interface_method() #3346

Conversation

@iwanowww
Copy link

@iwanowww iwanowww commented Apr 5, 2021

Turn resolved_method parameter into raw Method*.

Testing:

  • hs-tier1 - hs-tier6

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264732: Clean up LinkResolver::vtable_index_of_interface_method()

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3346/head:pull/3346
$ git checkout pull/3346

Update a local copy of the PR:
$ git checkout pull/3346
$ git pull https://git.openjdk.java.net/jdk pull/3346/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3346

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3346.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 5, 2021

👋 Welcome back vlivanov! 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
Copy link

@openjdk openjdk bot commented Apr 5, 2021

@iwanowww
The hotspot-runtime label was successfully added.

@iwanowww iwanowww marked this pull request as ready for review Apr 5, 2021
@openjdk openjdk bot added the rfr label Apr 5, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 5, 2021

@lfoltan
lfoltan approved these changes Apr 5, 2021
Copy link
Member

@lfoltan lfoltan left a comment

Looks good to me.
Lois

@openjdk
Copy link

@openjdk openjdk bot commented Apr 5, 2021

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

8264732: Clean up LinkResolver::vtable_index_of_interface_method()

Reviewed-by: lfoltan, 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 18 new commits pushed to the master branch:

  • 57f1e7d: 8264696: Multi-catch clause causes compiler exception because it uses the package-private supertype
  • 3d2b4cc: 8264864: Multiple byte tag not supported by ASN.1 encoding
  • ccefa5e: 8261625: Add Elements.isAutomaticModule(ModuleElement)
  • 8a23580: 8264428: Cleanup usages of StringBuffer in java.desktop
  • 308f679: 8264454: Jaxp unit test from open jdk needs to be improved
  • 5bd6c74: 8236127: Use value of --icon CLI option to set icon for exe installers
  • 81d35e4: 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop.
  • 04fa1ed: 8264848: [macos] libjvm.dylib linker warning due to macOS version mismatch
  • 214d6e2: 8263506: Make sun.net.httpserver.UnmodifiableHeaders unmodifiable
  • af13c64: 8264711: More runtime TRAPS cleanups
  • ... and 8 more: https://git.openjdk.java.net/jdk/compare/6e2b82a45f2f86c45fe62dcc6cf5cb979b8a5366...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 openjdk bot added the ready label Apr 5, 2021
@@ -728,7 +728,7 @@ C2V_END

C2V_VMENTRY_0(jint, getVtableIndexForInterfaceMethod, (JNIEnv* env, jobject, jobject jvmci_type, jobject jvmci_method))
Klass* klass = JVMCIENV->asKlass(jvmci_type);
methodHandle method(THREAD, JVMCIENV->asMethod(jvmci_method));
Method* method = JVMCIENV->asMethod(jvmci_method);

This comment has been minimized.

@coleenp

coleenp Apr 5, 2021

The reason that resolved_method was a methodHandle is in case of redefinition, we need to know if code is referring to this version of the method so that it's not deallocated. It's enough for one of the callers to create a methodHandle but passing the methodHandle will guarantee it. I'm not sure why you needed to make this change.

This comment has been minimized.

@iwanowww

iwanowww Apr 6, 2021
Author

I refactored getVtableIndexForInterfaceMethod and kept the handle in place while dereferencing it when passing into vtable_index_of_interface_method. Does it look better now?

This comment has been minimized.

@coleenp

coleenp Apr 6, 2021

Ok, this is fine. Be careful if you have new callers for this function.

@coleenp
coleenp approved these changes Apr 6, 2021
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Seems quite reasonable - though unsure of the motivation.

Do we need a NoSafepointVerifier in LinkResolver::vtable_index_of_interface_method to ensure the raw Method* remains valid?

Thanks,
David

Vladimir Ivanov and others added 2 commits Apr 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 7, 2021

@iwanowww this pull request can not be integrated into master 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 8264732.vtable_index_of_interface_method
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
@iwanowww
Copy link
Author

@iwanowww iwanowww commented Apr 7, 2021

Thinking more about it, methodHandle does look more natural than a raw Method* on LinkResolver.

What do you think about leaving LinkResolver as it is now and move vtable_index_of_interface_method implementation to InstanceKlass instead?


// First check in default method array
if (!intf_method->is_abstract() && this->default_methods() != NULL) {
int index = InstanceKlass::find_method_index(this->default_methods(),

This comment has been minimized.

@coleenp

coleenp Apr 7, 2021

Yes, please, this looks a lot better! Then linkResolver can deal with methodHandle consistently so it doesn't go away with redefinition, and this function is nice here with the other like functions.
It doesn't need InstanceKlass:: before find_method_index, or the this->'s.

This comment has been minimized.

@iwanowww

iwanowww Apr 8, 2021
Author

It doesn't need InstanceKlass:: before find_method_index, or the this->'s.

Fixed. It looked clearer to me to explicitly refer to this.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Refactoring looks good.

I second Coleen's comment about unnecessary classname qualification and use of this->

Thanks,
David

@iwanowww
Copy link
Author

@iwanowww iwanowww commented Apr 8, 2021

Thanks for the reviews, Lois, Coleen, and David.

@iwanowww
Copy link
Author

@iwanowww iwanowww commented Apr 9, 2021

/integrate

@openjdk openjdk bot closed this Apr 9, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 9, 2021

@iwanowww Since your change was applied there have been 28 commits pushed to the master branch:

  • b3782ea: 8264918: [JVMCI] getVtableIndexForInterfaceMethod doesn't check that type and method are related
  • f7a6c63: 8259822: [PPC64] Support the prefixed instruction format added in POWER10
  • a45733f: 8264779: Fix doclint warnings in java/nio
  • 3e57924: 8264885: Fix the code style of macro in aarch64_neon_ad.m4
  • 051c117: 8264923: PNGImageWriter.write_zTXt throws Exception with a typo
  • 1c6b113: 8264513: Cleanup CardTableBarrierSetC2::post_barrier
  • 666fd62: 8264881: Remove the old development option MemProfiling
  • 951f277: 8264874: Build interim-langtools for HotSpot only if Graal is enabled
  • 719f95e: 8260693: Provide the support for specifying a signer in keytool -genkeypair
  • 77b1673: 8256245: AArch64: Implement Base64 decoding intrinsic
  • ... and 18 more: https://git.openjdk.java.net/jdk/compare/6e2b82a45f2f86c45fe62dcc6cf5cb979b8a5366...master

Your commit was automatically rebased without conflicts.

Pushed as commit 33fa855.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants