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

8266822: Rename MetaspaceShared::is_old_class to be more explicit about what "old" means #3983

Closed

Conversation

calvinccheung
Copy link
Member

@calvinccheung calvinccheung commented May 11, 2021

Please review this simple patch for renaming the function from MetaspaceShared::is_old_class to MetaspaceShared::has_old_class_version. Also added some comment to the function.

Tests:

  • tier1, 2

Progress

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

Issue

  • JDK-8266822: Rename MetaspaceShared::is_old_class to be more explicit about what "old" means

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3983

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

Using diff file

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

@calvinccheung
Copy link
Member Author

calvinccheung commented May 11, 2021

/label remove hotspot
/label add hotspot-runtime

@bridgekeeper
Copy link

bridgekeeper bot commented May 11, 2021

👋 Welcome back ccheung! 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 bot commented May 11, 2021

@calvinccheung The label remore is not a valid label. These labels are valid:

  • serviceability
  • hotspot
  • sound
  • hotspot-compiler
  • kulla
  • i18n
  • shenandoah
  • jdk
  • javadoc
  • 2d
  • security
  • swing
  • hotspot-runtime
  • jmx
  • build
  • nio
  • beans
  • core-libs
  • compiler
  • net
  • hotspot-gc
  • hotspot-jfr
  • awt

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label May 11, 2021
@openjdk
Copy link

openjdk bot commented May 11, 2021

@calvinccheung
The hotspot-runtime label was successfully added.

@calvinccheung calvinccheung marked this pull request as ready for review May 11, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label May 11, 2021
@mlbridge
Copy link

mlbridge bot commented May 11, 2021

Webrevs

iklam
iklam approved these changes May 11, 2021
Copy link
Member

@iklam iklam left a comment

Looks good an trivial. Just a small nit for the comment text.

@@ -574,21 +574,25 @@ class CollectCLDClosure : public CLDClosure {
ClassLoaderData* cld_at(int index) { return _loaded_cld.at(index); }
};

// Check if a class or its super class/interface is old.
bool MetaspaceShared::is_old_class(InstanceKlass* ik) {
// Check if a class or its super class/interface has a version older than 50.
Copy link
Member

@iklam iklam May 11, 2021

Choose a reason for hiding this comment

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

How about "Check if a class or any of its supertypes has ...."

Copy link
Member Author

@calvinccheung calvinccheung May 11, 2021

Choose a reason for hiding this comment

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

@iklam Thanks for the review. I'll update the comment before integration.

Copy link
Contributor

@yminqi yminqi left a comment

Should we move has_old_class_version to Klass or InstanceKlass?

@coleenp
Copy link
Contributor

coleenp commented May 11, 2021

Calvin, thank you for doing this. I agree with Yumin that this seems to be a function of InstanceKlass.

@coleenp
Copy link
Contributor

coleenp commented May 11, 2021

Should we move has_old_class_version to Klass or InstanceKlass?

InstanceKlass makes more sense to me.

@calvinccheung
Copy link
Member Author

calvinccheung commented May 11, 2021

Calvin, thank you for doing this. I agree with Yumin that this seems to be a function of InstanceKlass.

Coleen and Yumin,
Thanks for the suggestion.
I've made the code changes. Running some tests before posting another webrev.

if (ik->major_version() < 50 /*JAVA_6_VERSION*/) {
return true;
}
if (has_old_class_version(ik->java_super())) {
Copy link
Member

@iklam iklam May 11, 2021

Choose a reason for hiding this comment

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

I think it's better to change this static method into an instance method, so you can call klass->has_old_class_version(). You'd also need to change the above "if" to

if (ik->java_super() != NULL && ik->super->has_old_class_version()) {

Copy link
Contributor

@coleenp coleenp May 11, 2021

Choose a reason for hiding this comment

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

Yes. I agree.

yminqi
yminqi approved these changes May 11, 2021
Copy link
Contributor

@yminqi yminqi left a comment

LGTM.

Copy link
Contributor

@coleenp coleenp left a comment

Thank you for making this change!

// without changing the old verifier, the verification constraint cannot be
// retrieved during dump time.
// Verification of archived old classes will be performed during run time.
bool InstanceKlass::has_old_class_version() {
Copy link
Contributor

@coleenp coleenp May 11, 2021

Choose a reason for hiding this comment

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

very small nit: Can you make this function 'const' ?

iklam
iklam approved these changes May 11, 2021
Copy link
Member

@iklam iklam left a comment

LGTM. Just a small nit. I think has_old_class_version can be declared as const.

@calvinccheung
Copy link
Member Author

calvinccheung commented May 11, 2021

/integrate

@openjdk
Copy link

openjdk bot commented May 11, 2021

@calvinccheung This PR has not yet been marked as ready for integration.

@calvinccheung calvinccheung changed the title 8266822: Rename MetaspaceShared::is_old_class to has_old_class_version 8266822: Rename MetaspaceShared::is_old_class to be more explicit about what "old" means May 11, 2021
@openjdk
Copy link

openjdk bot commented May 11, 2021

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

8266822: Rename MetaspaceShared::is_old_class to be more explicit about what "old" means

Reviewed-by: iklam, minqi, 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 28 new commits pushed to the master branch:

  • 616244f: 8266937: Remove Compile::reshape_address
  • 974b9f7: 8266773: Release VM is broken with GCC 9 after 8214237
  • f6c5a6b: 8266784: java/text/Collator/RuleBasedCollatorTest.java fails with jtreg 6
  • 1356116: 8266456: Replace direct TKit.run() calls with jdk.jpackage.test.Annotations.Test annotation
  • dfe8833: 8266783: java/lang/reflect/Proxy/DefaultMethods.java fails with jtreg 6
  • 995e956: 8266225: jarsigner is using incorrect security property to show weakness of certs
  • 0a12605: 8265448: (zipfs): Reduce read contention in ZipFileSystem
  • acf02ed: 8208237: Re-examine defmeth tests and update as needed
  • ac0287f: 8266770: Clean pending exception before running dynamic CDS dump
  • 7a0a57c: 8266820: micro java/nio/SelectorWakeup.java has wrong copyright header
  • ... and 18 more: https://git.openjdk.java.net/jdk/compare/de784312c340b4a4f4c4d11854bfbe9e9e826ea3...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 May 11, 2021
@calvinccheung
Copy link
Member Author

calvinccheung commented May 11, 2021

/integrate

@openjdk openjdk bot closed this May 11, 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 May 11, 2021
@openjdk
Copy link

openjdk bot commented May 11, 2021

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

  • 616244f: 8266937: Remove Compile::reshape_address
  • 974b9f7: 8266773: Release VM is broken with GCC 9 after 8214237
  • f6c5a6b: 8266784: java/text/Collator/RuleBasedCollatorTest.java fails with jtreg 6
  • 1356116: 8266456: Replace direct TKit.run() calls with jdk.jpackage.test.Annotations.Test annotation
  • dfe8833: 8266783: java/lang/reflect/Proxy/DefaultMethods.java fails with jtreg 6
  • 995e956: 8266225: jarsigner is using incorrect security property to show weakness of certs
  • 0a12605: 8265448: (zipfs): Reduce read contention in ZipFileSystem
  • acf02ed: 8208237: Re-examine defmeth tests and update as needed
  • ac0287f: 8266770: Clean pending exception before running dynamic CDS dump
  • 7a0a57c: 8266820: micro java/nio/SelectorWakeup.java has wrong copyright header
  • ... and 18 more: https://git.openjdk.java.net/jdk/compare/de784312c340b4a4f4c4d11854bfbe9e9e826ea3...master

Your commit was automatically rebased without conflicts.

Pushed as commit 57c6ba6.

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

@calvinccheung calvinccheung deleted the 8266822-rename-is-old-class branch May 12, 2021
@mlbridge
Copy link

mlbridge bot commented May 14, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Calvin,

On 12/05/2021 2:23 am, Calvin Cheung wrote:

Please review this simple patch for renaming the function from `MetaspaceShared::is_old_class` to `MetaspaceShared::has_old_class_version`. Also added some comment to the function.

I guess you missed the fact that I had changed the bug synopsis. Why not
call this needs_old_verifier (or something like that) so that the exact
meaning of this method is more clear?

David

@iklam
Copy link
Member

iklam commented May 14, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Calvin,

On 12/05/2021 2:23 am, Calvin Cheung wrote:

Please review this simple patch for renaming the function from MetaspaceShared::is_old_class to MetaspaceShared::has_old_class_version. Also added some comment to the function.

I guess you missed the fact that I had changed the bug synopsis. Why not
call this needs_old_verifier (or something like that) so that the exact
meaning of this method is more clear?

needs_old_verifier isn't clear, either. has_old_class_version(K) returns true if K is "new" but one of K's supertypes is "old". K itself does not "need the old verifier".

@mlbridge
Copy link

mlbridge bot commented May 17, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 15/05/2021 7:14 am, Ioi Lam wrote:

On Tue, 11 May 2021 22:47:44 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

Please review this simple patch for renaming the function from `MetaspaceShared::is_old_class` to `MetaspaceShared::has_old_class_version`. Also added some comment to the function.

Tests:
- [x] tier1, 2

Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:

declare has_old_class_version as a const method

_Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [hotspot-runtime-dev](mailto:hotspot-runtime-dev at mail.openjdk.java.net):_

Hi Calvin,

On 12/05/2021 2:23 am, Calvin Cheung wrote:

Please review this simple patch for renaming the function from `MetaspaceShared::is_old_class` to `MetaspaceShared::has_old_class_version`. Also added some comment to the function.

I guess you missed the fact that I had changed the bug synopsis. Why not
call this needs_old_verifier (or something like that) so that the exact
meaning of this method is more clear?

needs_old_verifier isn't clear, either. `has_old_class_version(K)` returns true if K is "new" but one of K's supertypes is "old". K itself does not "need the old verifier".

and k itself does not "have an old class version" so the current method
is even more misleadingly named!

How about inverting it and having has_post_Java_6_hierarchy: return true
if k and all its supertypes are post-Java-6 classfile version.

Seriously we must be able to name this method in a more meaningful way. :(

David
-----

@iklam
Copy link
Member

iklam commented May 17, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 15/05/2021 7:14 am, Ioi Lam wrote:

On Tue, 11 May 2021 22:47:44 GMT, Calvin Cheung wrote:

Please review this simple patch for renaming the function from MetaspaceShared::is_old_class to MetaspaceShared::has_old_class_version. Also added some comment to the function.
Tests:

  • tier1, 2

Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
declare has_old_class_version as a const method

Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [hotspot-runtime-dev](mailto:hotspot-runtime-dev at mail.openjdk.java.net):
Hi Calvin,
On 12/05/2021 2:23 am, Calvin Cheung wrote:

Please review this simple patch for renaming the function from MetaspaceShared::is_old_class to MetaspaceShared::has_old_class_version. Also added some comment to the function.

I guess you missed the fact that I had changed the bug synopsis. Why not
call this needs_old_verifier (or something like that) so that the exact
meaning of this method is more clear?

needs_old_verifier isn't clear, either. has_old_class_version(K) returns true if K is "new" but one of K's supertypes is "old". K itself does not "need the old verifier".

and k itself does not "have an old class version" so the current method
is even more misleadingly named!

How about inverting it and having has_post_Java_6_hierarchy: return true
if k and all its supertypes are post-Java-6 classfile version.

It's unclear whether hierarchy refers to the super types, subtypes, or both.

Seriously we must be able to name this method in a more meaningful way. :(

Trying to describe what the function does in a short and meaningful name is not always possible.

In this case, I think it's better to say what the function intends to do. InstanceKlass::can_be_verified_at_dumptime() will probably suffice. It's unlikely that this function will be used for any other purpose.

@mlbridge
Copy link

mlbridge bot commented May 17, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 17/05/2021 4:39 pm, Ioi Lam wrote:

On Tue, 11 May 2021 22:47:44 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

Please review this simple patch for renaming the function from `MetaspaceShared::is_old_class` to `MetaspaceShared::has_old_class_version`. Also added some comment to the function.

Tests:
- [x] tier1, 2

Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:

declare has_old_class_version as a const method

_Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [hotspot-runtime-dev](mailto:hotspot-runtime-dev at mail.openjdk.java.net):_

On 15/05/2021 7:14 am, Ioi Lam wrote:

On Tue, 11 May 2021 22:47:44 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

Please review this simple patch for renaming the function from `MetaspaceShared::is_old_class` to `MetaspaceShared::has_old_class_version`. Also added some comment to the function.
Tests:
- [x] tier1, 2

Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
declare has_old_class_version as a const method

_Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [hotspot-runtime-dev](mailto:hotspot-runtime-dev at mail.openjdk.java.net):_
Hi Calvin,
On 12/05/2021 2:23 am, Calvin Cheung wrote:

Please review this simple patch for renaming the function from `MetaspaceShared::is_old_class` to `MetaspaceShared::has_old_class_version`. Also added some comment to the function.

I guess you missed the fact that I had changed the bug synopsis. Why not
call this needs_old_verifier (or something like that) so that the exact
meaning of this method is more clear?

needs_old_verifier isn't clear, either. `has_old_class_version(K)` returns true if K is "new" but one of K's supertypes is "old". K itself does not "need the old verifier".

and k itself does not "have an old class version" so the current method
is even more misleadingly named!

How about inverting it and having has_post_Java_6_hierarchy: return true
if k and all its supertypes are post-Java-6 classfile version.

It's unclear whether hierarchy refers to the super types, subtypes, or both.

The hierarchy from a given class is always up.

Seriously we must be able to name this method in a more meaningful way. :(

Trying to describe what the function does in a short and meaningful name is not always possible.

In this case, I think it's better to say what the function intends to do. `InstanceKlass::can_be_verified_at_dumptime()` will probably suffice. It's unlikely that this function will be used for any other purpose.

That works for me.

Thanks,
David

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
4 participants