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

8262227: Change SystemDictionary::find() to return an InstanceKlass*. #2712

Closed
wants to merge 1 commit into from

Conversation

hseigel
Copy link
Member

@hseigel hseigel commented Feb 24, 2021

Please review this fix for JDK-8262227. This fix changes SystemDictionary::find() to return an InstanceKlass* to reduce InstanceKlass casts, it renames find() to find_instance_klass(), removes its unneeded TRAP parameter, and changes its callers as appropriate.

It also changed the get_java_...() methods, in thread.cpp, to take an InstanceKlass* parameter and removed their now unneeded TRAPS parameter.

The fix was tested with mach5 tiers 1 and 2 on Linux, Windows, and Mac OS, and tiers 3-5 on Linux x64 (still in progress).

Thanks to Coleen and David for their helpful suggestions.

Thanks, Harold


Progress

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

Issue

  • JDK-8262227: Change SystemDictionary::find() to return an InstanceKlass*.

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2712/head:pull/2712
$ git checkout pull/2712

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 24, 2021

👋 Welcome back hseigel! 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 openjdk bot added the rfr Pull request is ready for review label Feb 24, 2021
@openjdk
Copy link

openjdk bot commented Feb 24, 2021

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

  • hotspot

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.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Feb 24, 2021
@mlbridge
Copy link

mlbridge bot commented Feb 24, 2021

Webrevs

@@ -448,8 +448,7 @@ ciKlass* ciEnv::get_klass_by_name_impl(ciKlass* accessing_klass,
kls = SystemDictionary::find_constrained_instance_or_array_klass(sym, loader,
CHECK_AND_CLEAR_(fail_type));
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think SystemDictionary::find_constrained_instance_or_array_klass can also be changed to accept a Thread* instead TRAPS, since now it can no longer throw, and the thread is used only for Mutexes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Ioi, I'd like to do this in a separate RFE. Is that okay?

JDK_Version::set_runtime_vendor_vm_bug_url(get_java_runtime_vendor_vm_bug_url(THREAD));
InstanceKlass* ik = SystemDictionary::find_instance_klass(vmSymbols::java_lang_VersionProps(),
Handle(), Handle());
JDK_Version::set_java_version(get_java_version(ik));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The various get_java_xxx() functions all seem to do the same thing. I am wondering if they can be combined into a single utility function, so that you can do something like:

JDK_Version::set_runtime_name(get_version_info(ik, vmSymbols::java_version_name(),
                              java_version, sizeof(java_version)));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the get_java_* set of functions could be streamlined so that you pass in the symbol for the field you need. Also perhaps VersionProps could be a well-known class so we don't have to look it up. But this seems a future RFE. Even the current changes seem a little out-of-scope for this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this might be a useful change in a follow up RFE. What you have is good. You were going to change the lines anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Ioi, I'd like to do this cleanup in a separate RFE. Is that okay?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Harold, I think it's OK to do the cleanups in different RFEs.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup Harold! I'm more pleased with the TRAPS removal than the actual type change :)

Ref my comment in the bug report I'd overlooked that just because the return type of find changed, it didn't mean you have to change the type of the variable it was assigned to!

Thanks,
David

JDK_Version::set_runtime_vendor_vm_bug_url(get_java_runtime_vendor_vm_bug_url(THREAD));
InstanceKlass* ik = SystemDictionary::find_instance_klass(vmSymbols::java_lang_VersionProps(),
Handle(), Handle());
JDK_Version::set_java_version(get_java_version(ik));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the get_java_* set of functions could be streamlined so that you pass in the symbol for the field you need. Also perhaps VersionProps could be a well-known class so we don't have to look it up. But this seems a future RFE. Even the current changes seem a little out-of-scope for this change.

@openjdk
Copy link

openjdk bot commented Feb 24, 2021

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

8262227: Change SystemDictionary::find() to return an InstanceKlass*.

Reviewed-by: iklam, dholmes, coleenp

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

  • 228c285: 8261170: Upgrade to freetype 2.10.4
  • ded96dd: 8139348: Deprecate 3DES and RC4 in Kerberos
  • 5a9b701: 8258444: Clean up specifications of java.io.Reader.read(char[],int,int) in subclass overrides
  • 7d4f60b: 8260403: javap should be more robust in the face of invalid class files
  • 674be87: 8261203: Incorrectly escaped javadoc html with type annotations
  • 2eca17d: 8261457: test/langtools/tools/javac/T8187978 can fail if ArrayList class is modified
  • c54724d: 8257234: Add gz option to SA jmap to write a gzipped heap dump
  • aa35b42: 8261131: jcmd jmap dump should not accept gz option with no value
  • ebdc80e: 8252883: AccessDeniedException caused by delayed file deletion on Windows
  • f79c626: 8262296: Fix remaining doclint warnings in jdk.httpserver
  • ... and 5 more: https://git.openjdk.java.net/jdk/compare/65492129a906e200fae3aef2d965d590c22a0290...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 Pull request is ready to be integrated label Feb 24, 2021
Copy link
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of a cleanup than I thought it'd be. Very nice!

@hseigel
Copy link
Member Author

hseigel commented Feb 25, 2021

Thanks Ioi, Coleen, and David for the reviews!

/integrate

@openjdk openjdk bot closed this Feb 25, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 25, 2021
@openjdk
Copy link

openjdk bot commented Feb 25, 2021

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

  • 35c0a69: 8262416: ProblemList TestHeapDumpForLargeArray.java due to JDK-8262386
  • 228c285: 8261170: Upgrade to freetype 2.10.4
  • ded96dd: 8139348: Deprecate 3DES and RC4 in Kerberos
  • 5a9b701: 8258444: Clean up specifications of java.io.Reader.read(char[],int,int) in subclass overrides
  • 7d4f60b: 8260403: javap should be more robust in the face of invalid class files
  • 674be87: 8261203: Incorrectly escaped javadoc html with type annotations
  • 2eca17d: 8261457: test/langtools/tools/javac/T8187978 can fail if ArrayList class is modified
  • c54724d: 8257234: Add gz option to SA jmap to write a gzipped heap dump
  • aa35b42: 8261131: jcmd jmap dump should not accept gz option with no value
  • ebdc80e: 8252883: AccessDeniedException caused by delayed file deletion on Windows
  • ... and 6 more: https://git.openjdk.java.net/jdk/compare/65492129a906e200fae3aef2d965d590c22a0290...master

Your commit was automatically rebased without conflicts.

Pushed as commit 29c603f.

💡 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
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
4 participants