8298601: Refactor archiving of java.lang.Module objects #11715

iklam · 2022-12-19T05:28:39Z

This RFE is a prerequisite for JDK-8296344: Remove dependency on G1 for writing the CDS archive heap.

The main change of this RFE is in HeapShared::archive_reachable_objects_from(). We avoid the use of "archived objects" in the interface between CDS and the module system, and use integer "root indices" instead. (See note [1] in JDK-8298600.)

Also, for better encapsulation, the module-specific code has been moved from HeapShared::check_module_oop() to the three new functions in modules.cpp.

Besides the refactoring, this RFE also tightens up the code that deals with java.lang.Module oops and the corresponding C++ ModuleEntry objects.

As this is admittedly an obscured area of the CDS archive heap, I've added the following:

Assertions to make sure that when an oop is archived, the corresponding C++ object is also archived, and vice-versa.
More comments

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-8298601: Refactor archiving of java.lang.Module objects

Reviewers

Coleen Phillimore (@coleenp - Reviewer)
Calvin Cheung (@calvinccheung - Reviewer) ⚠️ Review applies to b7dd89ea

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11715

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

Using diff file

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

bridgekeeper · 2022-12-19T05:29:52Z

👋 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 · 2022-12-19T05:31:47Z

@iklam The following label will be automatically applied to this pull request:

hotspot-runtime

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.

mlbridge · 2022-12-19T05:35:41Z

Webrevs

coleenp

I suggested some name changes that might be helpful to people who don't already know this code, but the movement seems fine.

src/hotspot/share/classfile/moduleEntry.cpp

src/hotspot/share/classfile/modules.cpp

coleenp · 2022-12-20T13:47:13Z

src/hotspot/share/classfile/moduleEntry.cpp

+}
+
+#ifndef PRODUCT
+void ModuleEntry::validate_archived_module_entries() {


Can this have a check_module_entries name since it's called by a similarly sounding function name in Modules? It doesn't actually validate anything in the module entries except the count.

I renamed it to ModuleEntry::verify_archived_module_entries() to be consistent with Modules::verify_archived_modules().

coleenp · 2022-12-20T13:48:03Z

src/hotspot/share/classfile/moduleEntry.cpp

+
+  if (log_is_enabled(Info, cds, module)) {
+    ResourceMark rm;
+    log_info(cds, module)("Restored archived %s", debug_info());


I don't know if stringStream needs a resource mark so if it doesn't, you can just have one log line for these.

I used StringStream::as_string(bool c_heap = false) to return from debug_info(), so the result is resource allocated.

Anyway, I can see that this style may be confusing. I'll rewrite it to follow the print_on(st) style that's more common where the semantics are well understood. This will make the logging a little more tedious but I think it's OK.

calvinccheung

I have a few minor comments.

calvinccheung · 2022-12-22T02:20:54Z

src/hotspot/share/classfile/modules.hpp

@@ -53,8 +53,12 @@ class Modules : AllStatic {
  static void define_module(Handle module, jboolean is_open, jstring version,
                            jstring location, jobjectArray packages, TRAPS);

+  static bool check_module_oop(oop orig_module_obj);


I think this needs NOT_CDS_JAVA_HEAP_RETURN_(false) at the end.

calvinccheung · 2022-12-22T02:37:59Z

src/hotspot/share/classfile/moduleEntry.cpp

 ModuleEntry* ModuleEntry::allocate_archived_entry() const {
  assert(is_named(), "unnamed packages/modules are not archived");
  ModuleEntry* archived_entry = (ModuleEntry*)ArchiveBuilder::rw_region_alloc(sizeof(ModuleEntry));
  memcpy((void*)archived_entry, (void*)this, sizeof(ModuleEntry));
+  archived_entry->_archived_module_index = -2;


Why not use -1 as the initial value?

calvinccheung · 2022-12-22T03:02:24Z

src/hotspot/share/classfile/moduleEntry.cpp

+  if (loader_data()->is_boot_class_loader_data()) {
+    info.print("boot");
+  } else if (SystemDictionary::is_platform_class_loader(loader_data()->class_loader())) {
+    info.print("platform");
+  } else if (SystemDictionary::is_system_class_loader(loader_data()->class_loader())) {
+    info.print("system");
+  }


I think this could be simplified to
info.print(loader_data()->loader_name());

I changed the code to use ModuleEntry::print() instead, which internally calls loader_data()->loader_name_and_id().

calvinccheung

Updates look good. Thanks.

openjdk · 2022-12-23T00:35:39Z

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

8298601: Refactor archiving of java.lang.Module objects

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

…dule-oops

coleenp

Looks good.

iklam · 2023-01-04T04:02:13Z

Thanks @calvinccheung and @coleenp for the review.
/integrate

openjdk · 2023-01-04T04:03:58Z

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

77ff197: 7030853: JDK 7 Serializable Swing classes not compatible with JDK 6
38cfc59: 8299378: sprintf is deprecated in Xcode 14

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-04T04:04:16Z

@iklam Pushed as commit 82deb5c.

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

8298601: Refactor archiving of java.lang.Module objects

43b5390

openjdk bot added the rfr Pull request is ready for review label Dec 19, 2022

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Dec 19, 2022

coleenp reviewed Dec 20, 2022

View reviewed changes

calvinccheung reviewed Dec 22, 2022

View reviewed changes

review from @coleenp and @calvinccheung

b7dd89e

calvinccheung approved these changes Dec 23, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 23, 2022

Merge branch 'master' into 8298601-refactor-archiving-of-java-lang-Mo…

7ad443d

…dule-oops

coleenp approved these changes Jan 3, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 4, 2023

openjdk bot closed this Jan 4, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8298601: Refactor archiving of java.lang.Module objects #11715

8298601: Refactor archiving of java.lang.Module objects #11715

iklam commented Dec 19, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

mlbridge bot commented Dec 19, 2022 •

edited

Loading

coleenp left a comment

coleenp Dec 20, 2022

iklam Dec 22, 2022

coleenp Dec 20, 2022

iklam Dec 20, 2022

calvinccheung left a comment

calvinccheung Dec 22, 2022

iklam Dec 22, 2022

calvinccheung Dec 22, 2022

iklam Dec 22, 2022

calvinccheung Dec 22, 2022

iklam Dec 22, 2022

calvinccheung left a comment

openjdk bot commented Dec 23, 2022 •

edited

Loading

coleenp left a comment

iklam commented Jan 4, 2023

openjdk bot commented Jan 4, 2023

openjdk bot commented Jan 4, 2023

8298601: Refactor archiving of java.lang.Module objects #11715

8298601: Refactor archiving of java.lang.Module objects #11715

Conversation

iklam commented Dec 19, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 19, 2022

openjdk bot commented Dec 19, 2022

mlbridge bot commented Dec 19, 2022 • edited Loading

Webrevs

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvinccheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvinccheung left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 23, 2022 • edited Loading

coleenp left a comment

Choose a reason for hiding this comment

iklam commented Jan 4, 2023

openjdk bot commented Jan 4, 2023

openjdk bot commented Jan 4, 2023

iklam commented Dec 19, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 19, 2022 •

edited

Loading

openjdk bot commented Dec 23, 2022 •

edited

Loading