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

Jump to bottom

8293979: Resolve JVM_CONSTANT_Class references at CDS dump time #10330

Closed

iklam wants to merge 6 commits into openjdk:master from iklam:8293979-resolve-class-references-at-dumptime

Member

iklam commented Sep 19, 2022 •

edited by openjdk bot

Some JVM_CONSTANT_Class entries are guaranteed to resolve to the same value at both CDS dump time and run time:

Classes that are resolved during vmClasses::resolve_all(). These classes cannot be replaced by JVMTI agents at run time.
Supertypes -- at run time, a class C can be loaded from the CDS archive only if all of C's super types are also loaded from the CDS archive. Therefore, we know that a JVM_CONSTANT_Class reference to a supertype of C must resolved to the same value at both CDS dump time and run time.

By doing the resolution at dump time, we can speed up run time start-up by a little bit.

The ClassPrelinker class added by this PR will also be used in future REFs for pre-resolving other constant pool entries. The ultimate goal is to resolve invokedynamic and invokehandle so we can significantly improve the start-up time of features such as Lambda expressions and String concatenation. See JDK-8293336

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-8293979: Resolve JVM_CONSTANT_Class references at CDS dump time

Reviewers

Coleen Phillimore (@coleenp - Reviewer)
Calvin Cheung (@calvinccheung - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10330

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

Using diff file

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


          8293979: Resolve JVM_CONSTANT_Class references at CDS dump time

48a5652

bridgekeeper bot commented Sep 19, 2022

👋 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 bot commented Sep 19, 2022

@iklam 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 bot added the hotspot label

iklam marked this pull request as ready for review

September 19, 2022 04:39

openjdk bot added the rfr label

mlbridge bot commented Sep 19, 2022 •

edited

Webrevs

bridgekeeper bot commented Oct 17, 2022

@iklam This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Member Author

iklam commented Oct 17, 2022

Ping - any reviewers?

coleenp reviewed

View reviewed changes

Contributor

coleenp left a comment

I have questions.

src/hotspot/share/cds/classPrelinker.cpp

+                if (created) {
+                  InstanceKlass* super = ik->java_super();
+                  if (super != NULL) {
+                    add_one_vm_class(super);

Contributor

coleenp Oct 17, 2022

Do you care about order? Would it be better to add the superclasses before the instanceKlass?

Member Author

iklam Oct 18, 2022

The order doesn't matter since this is a hashtable. Also, I am trying to avoid walking up the hierarchy if a class is already in the table.

src/hotspot/share/cds/classPrelinker.cpp

+              }
+              #if INCLUDE_CDS_JAVA_HEAP
+              void ClassPrelinker::resolve_string(constantPoolHandle cp, int cp_index, TRAPS) {

Contributor

coleenp Oct 17, 2022

Is this function only needed in this cpp file? Can you make it static and defined before its caller? Are there other functions where this is true? Does it need to be a member function of ClassPrelinker?

Member Author

iklam Oct 18, 2022

I prefer to make such methods private, so it can access other private members of the same class.

Contributor

coleenp Oct 18, 2022

but this doesn't reference other private members of the class. The nice thing about it being a static non-member function is that you can easily see this without looking anywhere else. Also, it can be defined above the caller, which is where it's easy to find.
It's something that varies with developers but I like the style where you read a new file and see little function, little function, etc, leading up to the big function that calls them all.

src/hotspot/share/cds/classPrelinker.cpp Outdated

+                CPKlassSlot kslot = cp->klass_slot_at(cp_index);
+                int name_index = kslot.name_index();
+                Symbol* name = cp->symbol_at(name_index);

Contributor

coleenp Oct 17, 2022

Isn't this klass_name_at() ? I think these details should stay in constantPool.cpp.

Member Author

iklam Oct 18, 2022

I changed it to use klass_name_at().

src/hotspot/share/cds/classPrelinker.cpp Outdated

+                }
+              }
+              bool ClassPrelinker::can_archive_resolved_klass(ConstantPool* cp, int cp_index) {

Contributor

coleenp Oct 18, 2022

Can the other can_archive_resolved_klass be just above this one so we know why their names are the same? Also can_archive_vm_resolved_klass is too closely named.

Member Author

iklam Oct 18, 2022

I moved the two can_archive_resolved_klass() functions next to each other. I also folded can_archive_vm_resolved_klass into can_archive_resolved_klass(InstanceKlass*, Klass*), simplified it and improved the comments.

src/hotspot/share/cds/classPrelinker.cpp Outdated

+              Klass* ClassPrelinker::get_resolved_klass_or_null(ConstantPool* cp, int cp_index) {
+                if (cp->tag_at(cp_index).is_klass()) {
+                  CPKlassSlot kslot = cp->klass_slot_at(cp_index);

Contributor

coleenp Oct 18, 2022

This is another place that CPKlassSlot leaks out. It would be nice if these were in constantPool. Maybe this function belongs in the constant pool.

Member Author

iklam Oct 18, 2022

Changed to use ConstantPool::resolved_klass_at() instead.

src/hotspot/share/cds/classPrelinker.cpp Outdated

+              ClassPrelinker::ClassPrelinker() {
+                assert(_singleton == NULL, "must be");
+                _singleton = this;

Contributor

coleenp Oct 18, 2022

I'm not sure why you have this?

Member Author

iklam Oct 18, 2022

I was trying to put the states of the ClassPrelinker in its instance fields, but this is probably confusing. I'll change ClassPrelinker to AllStatic instead.

Contributor

coleenp Oct 18, 2022

It doesn't look like you did this.

src/hotspot/share/cds/classPrelinker.cpp Outdated Show resolved Hide resolved

iklam added 3 commits

October 17, 2022 22:49


          Merge branch 'master' into 8293979-resolve-class-references-at-dumptime

b00f266


          @coleenp comments

9bf8cb4


          fixed product build

980802d

coleenp reviewed

View reviewed changes

src/hotspot/share/oops/constantPool.cpp Show resolved Hide resolved

src/hotspot/share/cds/classPrelinker.hpp Outdated Show resolved Hide resolved

src/hotspot/share/cds/classPrelinker.cpp

+              }
+              #if INCLUDE_CDS_JAVA_HEAP
+              void ClassPrelinker::resolve_string(constantPoolHandle cp, int cp_index, TRAPS) {

Contributor

coleenp Oct 18, 2022

but this doesn't reference other private members of the class. The nice thing about it being a static non-member function is that you can easily see this without looking anywhere else. Also, it can be defined above the caller, which is where it's easy to find.
It's something that varies with developers but I like the style where you read a new file and see little function, little function, etc, leading up to the big function that calls them all.

src/hotspot/share/cds/classPrelinker.cpp Outdated

+              ClassPrelinker::ClassPrelinker() {
+                assert(_singleton == NULL, "must be");
+                _singleton = this;

Contributor

coleenp Oct 18, 2022

It doesn't look like you did this.

iklam added 2 commits

October 18, 2022 19:33


          Merge branch 'master' into 8293979-resolve-class-references-at-dumptime

d7676e1


          @coleenp comments: changed to AllStatic

df53a7b

coleenp approved these changes

View reviewed changes

Contributor

coleenp left a comment

Maybe the third read is the charm but this makes sense to me, and looks good.

openjdk bot commented Oct 20, 2022 •

edited

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

8293979: Resolve JVM_CONSTANT_Class references at CDS dump time

Reviewed-by: coleenp, ccheung

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

1164258: 8295124: Atomic::add to pointer type may return wrong value
d3eba85: 8295414: [Aarch64] C2: assert(false) failed: bad AD file
028e8b3: 8137022: Concurrent refinement thread adjustment and (de-)activation suboptimal
faa6b66: 8295715: Minimize disabled warnings in serviceability libs
de1e0c5: 8295719: Remove unneeded disabled warnings in jdk.sctp
9612cf9: 8295529: Add link to JBS to README.md
b37421e: 8295564: Norwegian Nynorsk Locale is missing formatting
6707bfb: 8029633: Raw inner class constructor ref should not perform diamond inference
7bc9692: 8294670: Enhanced switch statements have an implicit default which does not complete normally
95dd376: 8291914: generated constructors are considered compact when they shouldn't
... and 27 more: https://git.openjdk.org/jdk/compare/1553551d821d92e529116e6ce56846831b13f492...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 bot added the ready label

calvinccheung approved these changes

View reviewed changes

Member

calvinccheung left a comment

Looks good. Just one nit in constantPool.cpp.

src/hotspot/share/oops/constantPool.cpp

    
            @@ -388,44 +373,69 @@ void ConstantPool::remove_unshareable_info() {
          
                  resolved_references() != NULL ? resolved_references()->length() : 0);

                set_resolved_references(OopHandle());

                int num_klasses = 0;

                bool archived = false;

Member

calvinccheung Oct 20, 2022

I think this declaration could be moved to line 392 since it is only used in that case.

Member Author

iklam Oct 23, 2022

Fixed

Member Author

iklam commented Oct 23, 2022

Thanks to @coleenp and @calvinccheung for the review. Passed tiers1-2.
/integrate

openjdk bot commented Oct 23, 2022

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

7cbf672: 8295811: serviceability/sa/TestObjectAlignment.java fails on x86_32
adad59e: 8295762: [Vector API] Update generate_iota_indices for x86_32 after JDK-8293409
b5efa2a: 8294538: missing is_unloading() check in SharedRuntime::fixup_callers_callsite()
6acbdb5: 8295427: popframe004: report more details on error
902162c: 8295239: Refactor java/util/Formatter/Basic script into a Java native test launcher
f5dabf9: 8295088: Update External Spec page to show tabs for hosts
2181042: 8295375: debug agent class tracking should not piggy back on the cbClassPrepare() callback
f41711e: 8295650: JFR: jfr scrub should warn if an event type doesn't exist
0c13d66: 8295530: Update Zlib Data Compression Library to Version 1.2.13
15bebf9: 8295666: Linux x86 build fails after 8292591
... and 44 more: https://git.openjdk.org/jdk/compare/1553551d821d92e529116e6ce56846831b13f492...master

Your commit was automatically rebased without conflicts.

openjdk bot added the integrated label

openjdk bot closed this

openjdk bot removed ready rfr labels

openjdk bot commented Oct 23, 2022

@iklam Pushed as commit aad81f2.

💡 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