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

8261090: Store old classfiles in static CDS archive #3479

Closed

Conversation

calvinccheung
Copy link
Member

@calvinccheung calvinccheung commented Apr 14, 2021

Please review this RFE for storing old classfiles with major version < 50 in static CDS archive.
During static CDS dump time, old classes won't be verified/rewritten. They will be verified/rewritten during runtime.
Therefore, the _constMethod, _constants, and _cache of old classes must be stored in the RW region of the archive for runtime rewriting. The ConstantPool::remove_unshareable_info will be skipped during dump time and the ConstantPool::restore_unshareable_info will be skipped during runtime for old classes.

Passed tiers 1,2,3,4 tests on mach5.


Progress

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

Issue

  • JDK-8261090: Store old classfiles in static CDS archive

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3479

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

Using diff file

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

@calvinccheung
Copy link
Member Author

calvinccheung commented Apr 14, 2021

/label add hotspot-runtime
/label remove hotspot

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 14, 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 openjdk bot added the hotspot-runtime label Apr 14, 2021
@openjdk
Copy link

openjdk bot commented Apr 14, 2021

@calvinccheung
The hotspot-runtime label was successfully added.

@openjdk
Copy link

openjdk bot commented Apr 14, 2021

@calvinccheung The hotspot label was not set.

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

mlbridge bot commented Apr 14, 2021

Webrevs

@@ -733,7 +756,7 @@ bool MetaspaceShared::try_link_class(Thread* current, InstanceKlass* ik) {
ExceptionMark em(current);
Thread* THREAD = current; // For exception macros.
Arguments::assert_is_dumping_archive();
if (ik->is_loaded() && !ik->is_linked() &&
if (ik->is_loaded() && !ik->is_linked() && !MetaspaceShared::is_old_class(ik) &&
Copy link
Contributor

@jianglizhou jianglizhou Apr 14, 2021

Choose a reason for hiding this comment

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

It would be desirable to support the cases when verification is not enabled, which are not rare. For old classes, bytecode rewriting and class linking can still be done at dump time in those cases, with your current proposed change that includes old classes in the archive image. Otherwise, runtime would have to spend CPU cycles to do rewriting/linking for old classes when not truly necessary.

Copy link
Member Author

@calvinccheung calvinccheung Apr 15, 2021

Choose a reason for hiding this comment

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

I think that should be done in a separate RFE. It has security implication -- it may not be a good idea to implement a performance feature that you can get only by disabling verification. This will need more community input and security review.
What we have implemented in this PR is a compromise -- support old classes in CDS as much as we can, without breaking any security mechanisms.
There are also alternatives for further improvement. For example, we can modify the old verifier to store verification constraints in the CDS archive. This will require more work, but will preserve the Java security model.

@jianglizhou
Copy link
Contributor

jianglizhou commented Apr 14, 2021

Could you please provide performance numbers? It would be good to understand the performance benefits from archiving the old classes when verification and linking are deferred at runtime. Thanks.

@calvinccheung
Copy link
Member Author

calvinccheung commented Apr 15, 2021

Copy link
Member

@iklam iklam left a comment

Looks good to me. Some suggestions for comments and performance improvement.

@@ -284,8 +285,10 @@ bool Verifier::is_eligible_for_verification(InstanceKlass* klass, bool should_ve
// Can not verify the bytecodes for shared classes because they have
// already been rewritten to contain constant pool cache indices,
// which the verifier can't understand.
// Bytecodes for shared old classes can be verified because they have
// not been rewritten.
Copy link
Member

@iklam iklam Apr 19, 2021

Choose a reason for hiding this comment

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

Maybe put the above 2 lines under line 290, and change "can be" to "should be"?

// However, bytecodes for shared old classes should be verified because they have
// not been rewritten.

Copy link
Member Author

@calvinccheung calvinccheung Apr 19, 2021

Choose a reason for hiding this comment

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

Fixed.

if (!DumpSharedSpaces) {
assert(!klass->is_shared(), "archive methods must not be rewritten at run time");
if (klass->is_shared()) {
assert(!klass->is_rewritten(), "rewritten shared classes cannot be rewritten again");
Copy link
Member

@iklam iklam Apr 19, 2021

Choose a reason for hiding this comment

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

Also add

assert(MetaspaceShared::is_old_class(klass), "only shared old classes aren't rewritten");

Copy link
Member Author

@calvinccheung calvinccheung Apr 19, 2021

Choose a reason for hiding this comment

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

Fixed.

@@ -138,7 +138,11 @@ void ConstantPool::metaspace_pointers_do(MetaspaceClosure* it) {
log_trace(cds)("Iter(ConstantPool): %p", this);

it->push(&_tags, MetaspaceClosure::_writable);
it->push(&_cache);
if (!pool_holder()->is_rewritten()) {
it->push(&_cache, MetaspaceClosure::_writable);
Copy link
Member

@iklam iklam Apr 19, 2021

Choose a reason for hiding this comment

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

This shouldn't be necessary. All ConstantPoolCache are allocated in RW space by default.

Copy link
Member Author

@calvinccheung calvinccheung Apr 19, 2021

Choose a reason for hiding this comment

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

Fixed.

@@ -51,7 +51,7 @@ inline InstanceKlass* klassVtable::ik() const {
}

bool klassVtable::is_preinitialized_vtable() {
return _klass->is_shared() && !MetaspaceShared::remapped_readwrite();
return _klass->is_shared() && !MetaspaceShared::remapped_readwrite() && !MetaspaceShared::is_old_class(ik());
Copy link
Member

@iklam iklam Apr 19, 2021

Choose a reason for hiding this comment

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

Maybe we can avoid calling MetaspaceShared::is_old_class() by adding an extra bit into klass.hpp:

#if INCLUDE_CDS
  // Flags of the current shared class.
  u2     _shared_class_flags;
  enum {
    _archived_lambda_proxy_is_available = 2,
    _has_value_based_class_annotation = 4,
+   _is_shared_old_klass = 8
  };
#endif

Copy link
Member Author

@calvinccheung calvinccheung Apr 20, 2021

Choose a reason for hiding this comment

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

I've added the bit and it will be set in MetaspaceShared::linking_required.
Also replaced calling MetaspaceShared::is_old_class() with k->is_shared_old_klass() in several places.

Thanks for your review.

iklam
iklam approved these changes Apr 21, 2021
Copy link
Member

@iklam iklam left a comment

LGTM

@openjdk
Copy link

openjdk bot commented Apr 21, 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:

8261090: Store old classfiles in static CDS archive

Reviewed-by: iklam, minqi

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

  • 9e7c748: 8265700: Regularize throws clauses in BigDecimal
  • 7116321: 8254565: JFR: Incorrect verification of mirror events
  • f45d460: 8265017: runtime/HiddenClasses/StressHiddenClasses.java timed out on Win* OCI
  • 0136c89: 8265490: Unterminated string passed to FindClass() in hotspot test
  • 325edbc: 8265450: Merge PreservedMarksSet::restore code paths
  • b337f63: 8037397: RegEx pattern matching loses character class after intersection (&&) operator
  • 07a7510: 8200145: Conditional expression mistakenly treated as standalone
  • 41fc7dd: 8199079: Test javax/swing/UIDefaults/6302464/bug6302464.java is unstable
  • 45c474a: 8168408: Test java/awt/Focus/ActualFocusedWindowTest/ActualFocusedWindowBlockingTest.java fails intermittentently on windows
  • 18ee419: 8198422: Test java/awt/font/StyledMetrics/BoldSpace.java is unstable
  • ... and 63 more: https://git.openjdk.java.net/jdk/compare/ff4997014fe5462dca2b313f3f483400ffee5b62...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 21, 2021
!klass->is_shared() &&
// However, bytecodes for shared old classes can be verified because
// they have not been rewritten.
(!(klass->is_shared() && klass->is_rewritten())) &&
Copy link
Contributor

@yminqi yminqi Apr 21, 2021

Choose a reason for hiding this comment

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

the outmost brackets are not necessary. Could your remove it?

Copy link
Member Author

@calvinccheung calvinccheung Apr 21, 2021

Choose a reason for hiding this comment

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

Fixed. I also removed the extra include of metaspaceShared.hpp.

// create archive with class list
OutputAnalyzer output = TestCommon.dump(appJar, appClasses, "-Xlog:class+load,cds=debug,verification=trace");
TestCommon.checkExecReturn(output, 0,
dynamicMode ? true : false,
Copy link
Contributor

@yminqi yminqi Apr 21, 2021

Choose a reason for hiding this comment

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

Why use a ternary operator, is it the variable itself OK here?

Copy link
Member Author

@calvinccheung calvinccheung Apr 21, 2021

Choose a reason for hiding this comment

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

I prefer to leave it as is because the third arg to TestCommon.checkExecReturn() is about whether or not to look for the strings in the subsequent args.

Thanks for your review.

yminqi
yminqi approved these changes Apr 21, 2021
Copy link
Contributor

@yminqi yminqi left a comment

LGTM, only small nits.

@calvinccheung
Copy link
Member Author

calvinccheung commented Apr 22, 2021

/integrate

@openjdk openjdk bot closed this Apr 22, 2021
@openjdk openjdk bot added the integrated label Apr 22, 2021
@openjdk openjdk bot removed ready rfr labels Apr 22, 2021
@openjdk
Copy link

openjdk bot commented Apr 22, 2021

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

  • 159f5e1: 8263636: Add --disable-registry option to jhsdb debugd
  • 6d49cc3: 8265453: SafepointMechanism::should_process() should receive JavaThread*
  • fa82d47: 8264514: HexFormat implementation tweaks
  • e16d568: 8265218: trace_method_handle_stub fails to find calling frame on x86
  • 86bd44f: 8257831: Suspend with handshakes
  • 28af31d: 8263668: Update java.time to use instanceof pattern variable
  • a93d911: 8265607: Avoid decrementing when no Symbol was created in ~SignatureStream
  • aa29784: 8265332: gtest/LargePageGtests.java OOMEs on -XX:+UseSHM cases
  • 33b6378: 8265101: Remove unnecessary functions in os*.inline.hpp
  • 28c35ae: 8259288: Debug build failure with clang-10 due to -Wimplicit-int-float-conversion
  • ... and 74 more: https://git.openjdk.java.net/jdk/compare/ff4997014fe5462dca2b313f3f483400ffee5b62...master

Your commit was automatically rebased without conflicts.

Pushed as commit 9499175.

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

@calvinccheung calvinccheung deleted the 8261090-store-old-classfiles branch Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime integrated
4 participants