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

8244778: Archive full module graph in CDS #80

Closed
wants to merge 10 commits into from

Conversation

iklam
Copy link
Member

@iklam iklam commented Sep 8, 2020

This is the same patch as 8244778-archive-full-module-graph.v03 published in hotspot-runtime-dev@openjdk.java.net.

The rest of the review will continue on GitHub. I will add new commits to respond to comments to the above e-mail.


Progress

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

Issue

Reviewers

Download

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

@iklam
Copy link
Member Author

iklam commented Sep 8, 2020

/cc core-libs

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 8, 2020

👋 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 openjdk bot added the core-libs core-libs-dev@openjdk.org label Sep 8, 2020
@openjdk
Copy link

openjdk bot commented Sep 8, 2020

@iklam
The core-libs label was successfully added.

@openjdk
Copy link

openjdk bot commented Sep 8, 2020

@iklam The following labels will be automatically applied to this pull request: build hotspot security.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label (add|remove) "label" command.

@openjdk openjdk bot added security security-dev@openjdk.org hotspot hotspot-dev@openjdk.org build build-dev@openjdk.org labels Sep 8, 2020
@iklam
Copy link
Member Author

iklam commented Sep 8, 2020

/reviewer add lfoltan,coleenp,alanb,mchung

@openjdk
Copy link

openjdk bot commented Sep 8, 2020

@iklam
Reviewer lfoltan successfully added.

Reviewer coleenp successfully added.

Reviewer alanb successfully added.

Reviewer mchung successfully added.

@iklam iklam force-pushed the 8244778-archive-full-module-graph branch from eaa9125 to 89f3327 Compare September 8, 2020 16:24
@openjdk
Copy link

openjdk bot commented Sep 8, 2020

@iklam This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8244778: Archive full module graph in CDS

Reviewed-by: erikj, coleenp, lfoltan, redestad, alanb, mchung
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

There are currently no new commits on the master branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate 998ce78e530ccb52a76369ea6f5bdd9a3f90601c.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 8, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 8, 2020

Webrevs

@iklam
Copy link
Member Author

iklam commented Sep 8, 2020

In response to Lois Foltain's comments on hotspot-runtime-dev@openjdk.java.net:

Minor nit in moduleEntry.cpp & packageEntry.cpp when dealing with the ModuleEntry's reads list and a PackageEntry's exports list. The names of the methods to write and read those arrays is somewhat confusing.

ModuleEntry::write_archived_entry_array
ModuleEntry::read_archived_entry_array

At first I thought you were reading/writing an array of archived entries, not the array within an archived entry itself. I was trying to think of a better name. Please consider adding a comment at line #400 & line #417 ahead of those methods in moduleEntry.cpp to indicate that they are used for both reading/writing a ModuleEntry's reads list and a PackageEntry's exports list.

I renamed the functions to ModuleEntry's::write_growable_array and ModuleEntry::restore_growable_array, and added comments as you suggested. See commit 4f90e77

// This function is used to archive ModuleEntry::_reads and PackageEntry::_qualified_exports.
// GrowableArray cannot be directly archived, as it needs to be expandable at runtime.
// Write it out as an Array, and convert it back to GrowableArray at runtime.
Array<ModuleEntry*>* ModuleEntry::write_growable_array(GrowableArray<ModuleEntry*>* array) {

A question about this because a user's program can define modules post module initialization via ModuleDescriptor.newModule(). See for example, tests within open/test/hotspot/jtreg/runtime/module/AccessCheck. So all of these tests would trigger check_cds_restrictions() if -Xshare:dump was turned on. Is that a concern?

Arbitrary user code cannot be executed during -Xshare:dump. The only way to do it is to use a JVMTI agent, which requires specifying -XX:+AllowArchivingWithJavaAgent. You can see an example in the GCDuringDump.java test. If the agent tries to define an extra module, it will get an UnsupportedOperationException thrown by check_cds_restrictions().

@iklam
Copy link
Member Author

iklam commented Sep 8, 2020

/label remove build,hotspot

@iklam
Copy link
Member Author

iklam commented Sep 8, 2020

/cc hotspot-runtime

@openjdk openjdk bot removed the build build-dev@openjdk.org label Sep 8, 2020
@openjdk
Copy link

openjdk bot commented Sep 8, 2020

@iklam
The build label was successfully removed.

The hotspot label was successfully removed.

@openjdk openjdk bot added hotspot-runtime hotspot-runtime-dev@openjdk.org and removed hotspot hotspot-dev@openjdk.org labels Sep 8, 2020
@openjdk
Copy link

openjdk bot commented Sep 8, 2020

@iklam
The hotspot-runtime label was successfully added.

@iklam
Copy link
Member Author

iklam commented Sep 8, 2020

/label remove security

@openjdk openjdk bot removed the security security-dev@openjdk.org label Sep 8, 2020
@openjdk
Copy link

openjdk bot commented Sep 8, 2020

@iklam
The security label was successfully removed.

Copy link
Member

@erikj79 erikj79 left a comment

Choose a reason for hiding this comment

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

Build changes look good.

src/hotspot/share/classfile/classLoaderDataShared.hpp Outdated Show resolved Hide resolved
src/hotspot/share/classfile/modules.cpp Outdated Show resolved Hide resolved
src/hotspot/share/classfile/classLoaderDataShared.cpp Outdated Show resolved Hide resolved
Copy link
Member

@lfoltan lfoltan left a comment

Choose a reason for hiding this comment

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

Thanks Ioi for addressing my review comments. Overall, looks great!

src/hotspot/share/classfile/moduleEntry.cpp Show resolved Hide resolved
src/hotspot/share/oops/instanceKlass.cpp Outdated Show resolved Hide resolved
@coleenp
Copy link
Contributor

coleenp commented Sep 9, 2020

Ok thanks! So many emails ...

Copy link
Member

@cl4es cl4es left a comment

Choose a reason for hiding this comment

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

Excellent work!

Only a few minor comments inline, which you can choose to ignore.

assert(DumpSharedSpaces, "must be");
assert_valid(loader_data);
if (loader_data != NULL) {
// We can't create hashtables at dump time because the hashcode dependes on the
Copy link
Member

Choose a reason for hiding this comment

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

dependes -> depends

if (loader_data != NULL) {
// We can't create hashtables at dump time because the hashcode dependes on the
// address of the Symbols, which may be relocated at run time due to ASLR.
// So we store the packages/modules in a Arrays. At run time, we create
Copy link
Member

Choose a reason for hiding this comment

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

run time -> runtime
a Arrays -> Arrays

@@ -4821,7 +4826,6 @@ bool JavaClasses::is_supported_for_archiving(oop obj) {
Klass* klass = obj->klass();

if (klass == SystemDictionary::ClassLoader_klass() || // ClassLoader::loader_data is malloc'ed.
klass == SystemDictionary::Module_klass() || // Module::module_entry is malloc'ed
// The next 3 classes are used to implement java.lang.invoke, and are not used directly in
// regular Java code. The implementation of java.lang.invoke uses generated anonymoys classes
Copy link
Member

Choose a reason for hiding this comment

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

pre-existing: anonymoys

assert(UseSharedSpaces && MetaspaceShared::use_full_module_graph(), "must be");

// We don't want the classes used by the archived full module graph to be redefined by JVMTI.
// Luckily, such classes are loaded in the JVMTI "early" phase, and CDS is disable if a JVMTI
Copy link
Member

Choose a reason for hiding this comment

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

disabled

{"java/lang/Character$CharacterCache", "archivedCache"},
{"java/util/jar/Attributes$Name", "KNOWN_NAMES"},
{"sun/util/locale/BaseLocale", "constantBaseLocales"},
{"java/lang/Integer$IntegerCache", 0, "archivedCache"},
Copy link
Member

Choose a reason for hiding this comment

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

Could the changes here be simplified or clarified? I think the new field should be a bool, or we could instead introduce a new array for the fields archived only when archiving the full module graph (the field is ignored on iteration over closed_archive_subgraph_entry_fields anyhow)

Copy link
Member Author

Choose a reason for hiding this comment

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

I split out the new fields into a separate array as you suggested. Also fixed the typos you found. See commit e987110.

@iklam
Copy link
Member Author

iklam commented Sep 13, 2020

/integrate

@openjdk openjdk bot closed this Sep 13, 2020
@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 Sep 13, 2020
@openjdk
Copy link

openjdk bot commented Sep 13, 2020

@iklam Pushed as commit 03a4df0.

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

@iklam iklam deleted the 8244778-archive-full-module-graph branch February 18, 2021 00:46
lewurm added a commit to lewurm/openjdk that referenced this pull request Oct 6, 2021
Restore looks like this now:
```
  0x0000000106e4dfcc:   movk    x9, #0x5e4, lsl openjdk#16
  0x0000000106e4dfd0:   movk    x9, #0x1, lsl openjdk#32
  0x0000000106e4dfd4:   blr x9
  0x0000000106e4dfd8:   ldp x2, x3, [sp, openjdk#16]
  0x0000000106e4dfdc:   ldp x4, x5, [sp, openjdk#32]
  0x0000000106e4dfe0:   ldp x6, x7, [sp, openjdk#48]
  0x0000000106e4dfe4:   ldp x8, x9, [sp, openjdk#64]
  0x0000000106e4dfe8:   ldp x10, x11, [sp, openjdk#80]
  0x0000000106e4dfec:   ldp x12, x13, [sp, openjdk#96]
  0x0000000106e4dff0:   ldp x14, x15, [sp, openjdk#112]
  0x0000000106e4dff4:   ldp x16, x17, [sp, openjdk#128]
  0x0000000106e4dff8:   ldp x0, x1, [sp], openjdk#144
  0x0000000106e4dffc:   ldp xzr, x19, [sp], openjdk#16
  0x0000000106e4e000:   ldp x22, x23, [sp, openjdk#16]
  0x0000000106e4e004:   ldp x24, x25, [sp, openjdk#32]
  0x0000000106e4e008:   ldp x26, x27, [sp, openjdk#48]
  0x0000000106e4e00c:   ldp x28, x29, [sp, openjdk#64]
  0x0000000106e4e010:   ldp x30, xzr, [sp, openjdk#80]
  0x0000000106e4e014:   ldp x20, x21, [sp], openjdk#96
  0x0000000106e4e018:   ldur    x12, [x29, #-24]
  0x0000000106e4e01c:   ldr x22, [x12, openjdk#16]
  0x0000000106e4e020:   add x22, x22, #0x30
  0x0000000106e4e024:   ldr x8, [x28, openjdk#8]
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
5 participants