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

8276126: Dump time class transformation causes heap objects of non-boot classes to be archived #6484

Conversation

calvinccheung
Copy link
Member

@calvinccheung calvinccheung commented Nov 20, 2021

During CDS static dump, if a class loaded by a boot class loader is transformed by a java agent, the transformed class is mistakenly identified as a class loaded by a custom loader. This would cause archiving of the heap objects to fail.

The proposed patch corrects the identification of the transformed class and will not include it in the archive.
Also, if a transformed class is detected during dump time, the archiving of the heap objects will be disabled.

Testing: Oracle CI tiers 1-3, tier4 in progress.


Progress

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

Issue

  • JDK-8276126: Dump time class transformation causes heap objects of non-boot classes to be archived

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6484

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

Using diff file

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

@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Nov 20, 2021

/label add hotspot-runtime

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 20, 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 Nov 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 20, 2021

@calvinccheung
The hotspot-runtime label was successfully added.

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

@mlbridge mlbridge bot commented Nov 20, 2021

Webrevs

Otherwise, some appcds tests would fail in tier4 testing with -Xcomp
…ate_from_stream() to ClassLoader::record_result()
iklam
iklam approved these changes Nov 30, 2021
Copy link
Member

@iklam iklam left a comment

LGTM. One small nit about unused parameter.

@@ -227,7 +229,8 @@ void ClassLoaderExt::setup_search_paths(JavaThread* current) {
ClassLoaderExt::setup_app_search_path(current);
}

void ClassLoaderExt::record_result(const s2 classpath_index, InstanceKlass* result) {
void ClassLoaderExt::record_result(const s2 classpath_index, InstanceKlass* result,
const ClassFileStream* stream, bool redefined) {
Copy link
Member

@iklam iklam Nov 30, 2021

Choose a reason for hiding this comment

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

The stream argument is not used.

Copy link
Member Author

@calvinccheung calvinccheung Nov 30, 2021

Choose a reason for hiding this comment

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

Thanks for spotting this. I've fixed it.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 30, 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:

8276126: Dump time class transformation causes heap objects of non-boot classes to be archived

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

  • 51d6d7a: 8266839: Enable pandoc on macosx-aarch64 at Oracle
  • 0dfb3a7: 8268582: javadoc throws NPE with --ignore-source-errors option
  • f41e768: 8277762: Allow configuration of HOTSPOT_BUILD_USER
  • a363b7b: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST
  • 9b3e672: 8278014: [vectorapi] Remove test run script
  • 1e9ed54: 8193682: Infinite loop in ZipOutputStream.close()
  • abaa073: 8277946: NMT: Deprecate and remove VM.native_memory shutdown jcmd command option
  • 37ff7f3: 8277866: gc/epsilon/TestMemoryMXBeans.java failed with wrong initial heap size
  • 8d7958e: 8277981: String Deduplication table is never cleaned up due to bad dead_factor_for_cleanup
  • bc6dce1: 8277736: G1: Allow forced evacuation failure of first N regions in collection set
  • ... and 191 more: https://git.openjdk.java.net/jdk/compare/d5e47d6b84514edde23a8baff8c2274e5b3ca6bb...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 Nov 30, 2021
@@ -245,4 +246,21 @@ void ClassLoaderExt::record_result(const s2 classpath_index, InstanceKlass* resu
}
result->set_shared_classpath_index(classpath_index);
result->set_shared_class_loader_type(classloader_type);
#if INCLUDE_CDS_JAVA_HEAP
if (DumpSharedSpaces && AllowArchivingWithJavaAgent && classloader_type == ClassLoader::BOOT_LOADER &&
Copy link
Contributor

@yminqi yminqi Dec 1, 2021

Choose a reason for hiding this comment

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

the function is call during dump time (the assert at first line of this function check that) so DumpSharedSpace can be removed.
I am a little confused by this part: If 'redefined' is 'true', The 'redefined' only should be enough for set disable_writing() whether other conditions stands or not?

Copy link
Member Author

@calvinccheung calvinccheung Dec 1, 2021

Choose a reason for hiding this comment

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

The Arguments::assert_is_dumping_archive() includes both static and dynamic dumping. The archiving of heap objects is performed only during static dumping. So the check for DumpShareSpaces is needed.

The redefined flag alone is not enough to disable writing heap objects to archive. If a class loaded by an AppClassLoader and is redefined, it should not disable archiving heap objects. We currently have some test cases under runtime/cds/appcds/javaldr to test that.

Copy link
Contributor

@yminqi yminqi Dec 1, 2021

Choose a reason for hiding this comment

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

Thanks for the explanation.

yminqi
yminqi approved these changes Dec 1, 2021
Copy link
Contributor

@yminqi yminqi left a comment

LGTM.

@calvinccheung
Copy link
Member Author

@calvinccheung calvinccheung commented Dec 2, 2021

Thanks @iklam and @yminqi for the review.
/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2021

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

  • 7696897: 8276837: [macos]: Error when signing the additional launcher
  • 3d98ec1: 8273056: java.util.random does not correctly sample exponential or Gaussian distributions
  • b79554b: 8278130: Failure in jdk/javadoc/tool/CheckManPageOptions.java after JDK-8274639
  • ea905bd: 8277924: Small tweaks to foreign function and memory API
  • e002bfe: 8278049: G1: add precondition to set_remainder_to_point_to_start
  • 16cfbc4: 8278071: typos in MemorySegment::set, MemorySegment::setAtIndex javadoc
  • 84ca14d: 8277194: applications/runthese/RunThese30M.java crashes with jfrSymbolTable.cpp:305 assert(_instance != null)
  • 103da8f: 8274639: Provide a way to disable warnings for cross-modular links
  • 088b244: 8251216: Implement MD5 intrinsics on AArch64
  • a093cdd: 8276657: XSLT compiler tries to define a class with empty name
  • ... and 205 more: https://git.openjdk.java.net/jdk/compare/d5e47d6b84514edde23a8baff8c2274e5b3ca6bb...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Dec 2, 2021

@calvinccheung Pushed as commit d2b16c8.

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

@calvinccheung calvinccheung deleted the 8276126-dump-time-class-transform-heap-objects branch Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime integrated
3 participants