8271420: Extend CDS custom loader support to Windows platform #6202

yminqi · 2021-11-02T00:41:13Z

Currently CDS does not support custom loaders on Windows, this patch extends the support to Windows 64 bit platform.

Tests: mach5 tier1-4

Thanks
Yumin

Progress

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

Issue

JDK-8271420: Extend CDS custom loader support to Windows platform

Reviewers

Ioi Lam (@iklam - Reviewer)
Calvin Cheung (@calvinccheung - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6202

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

Using diff file

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

bridgekeeper · 2021-11-02T00:42:51Z

👋 Welcome back minqi! 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 · 2021-11-02T00:44:33Z

@yminqi The following labels will be automatically applied to this pull request:

core-libs
hotspot

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 pull request command.

yminqi · 2021-11-02T00:52:07Z

/label remove core-libs
/label remove hotspot
/label add hotspot-runtime

openjdk · 2021-11-02T00:53:19Z

@yminqi
The core-libs label was successfully removed.

openjdk · 2021-11-02T00:55:08Z

@yminqi
The hotspot label was successfully removed.

openjdk · 2021-11-02T00:57:40Z

@yminqi
The hotspot-runtime label was successfully added.

mlbridge · 2021-11-02T15:47:33Z

Webrevs

01: Full - Incremental (66f7c4dc)
00: Full (9460cafb)

iklam

Hi Yumin, the changes for custom class loaders look good. However, I think you should revert the INCLUDE_CDS_JAVA_HEAP changes since that's unrelated to custom class loaders.

iklam · 2021-11-02T15:55:57Z

src/hotspot/share/cds/heapShared.cpp

@@ -280,10 +280,10 @@ oop HeapShared::archive_object(oop obj) {
    return ao;
  }

-  int len = obj->size();
+  int len = (int)obj->size();


This code is used by INCLUDE_CDS_JAVA_HEAP and is unrelated to custom class loaders.

iklam · 2021-11-02T15:56:29Z

src/hotspot/share/cds/classListParser.cpp

@@ -446,7 +446,7 @@ void ClassListParser::error(const char* msg, ...) {
 // This function is used for loading classes for customized class loaders
 // during archive dumping.
 InstanceKlass* ClassListParser::load_class_from_source(Symbol* class_name, TRAPS) {
-#if !(defined(_LP64) && (defined(LINUX) || defined(__APPLE__)))
+#if !(defined(_LP64) && (defined(LINUX) || defined(__APPLE__) || defined(_WINDOWS)))


The comments in the two lines below need to be updated.

iklam · 2021-11-02T15:57:18Z

src/hotspot/share/utilities/macros.hpp

@@ -595,7 +595,7 @@
 #define COMPILER_HEADER(basename)        XSTR(COMPILER_HEADER_STEM(basename).hpp)
 #define COMPILER_HEADER_INLINE(basename) XSTR(COMPILER_HEADER_STEM(basename).inline.hpp)

-#if INCLUDE_CDS && INCLUDE_G1GC && defined(_LP64) && !defined(_WINDOWS)
+#if INCLUDE_CDS && INCLUDE_G1GC && defined(_LP64)


This change is unrelated to custom class loaders.

iklam · 2021-11-02T15:58:22Z

src/hotspot/share/classfile/stringTable.cpp

@@ -766,7 +766,7 @@ void StringTable::write_to_archive(const DumpedInternedStrings* dumped_interned_
  assert(HeapShared::can_write(), "must be");

  _shared_table.reset();
-  CompactHashtableWriter writer(_items_count, ArchiveBuilder::string_stats());
+  CompactHashtableWriter writer((int)_items_count, ArchiveBuilder::string_stats());


This code is used by INCLUDE_CDS_JAVA_HEAP and is unrelated to custom class loaders.

calvinccheung · 2021-11-02T18:25:54Z

src/hotspot/share/cds/heapShared.cpp

@@ -280,10 +280,10 @@ oop HeapShared::archive_object(oop obj) {
    return ao;
  }

-  int len = obj->size();
+  int len = (int)obj->size();


obj->size() already returns an int so I don't think the typecast is needed.
I agree with Ioi that this file shouldn't be changed.

oop.hpp:105
// Returns the actual oop size of the object
inline size_t size();

I will revert the change since it is not related to custom loader.

calvinccheung · 2021-11-02T18:26:03Z

test/hotspot/jtreg/runtime/cds/appcds/test-classes/CustomAppLoader.java

@@ -40,7 +40,7 @@
    // args[2...] = arguments for the main class
    public static void main(String args[]) throws Throwable {
        File f = new File(args[0]);
-        URL[] classLoaderUrls = new URL[] {new URL("file://" + f.getCanonicalPath())};
+        URL[] classLoaderUrls = new URL[] {f.toURI().toURL()};


Not sure if the above can handle relative path.
It is better to set f as follows:
File f = Path.of(args[0]).toRealPath().toFile();
Before the above, there should be a check if the length of args is 1.

calvinccheung

I have couple of comments.

openjdk · 2021-11-03T21:31:27Z

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

8271420: Extend CDS custom loader support to Windows platform

Reviewed-by: iklam, 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 50 new commits pushed to the master branch:

f3320d2: 8276588: Change "ccc" to "CSR" in HotSpot sources
32895ac: 8275650: Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11
c7f070f: 8276208: vmTestbase/nsk/jdb/repeat/repeat001/repeat001.java fails with "AssertionError: Unexpected output"
684edbb: 8273922: (fs) UserDefinedFileAttributeView doesn't handle file names that are just under the MAX_PATH limit (win)
7115892: 8276401: Use blessed modifier order in java.net.http
0ef8cbe: 8276385: Re-run blessed-modifier-order script on java.desktop and jdk.accessibility
724bf3b: 8275604: Zero: Reformat opclabels_data
61cb4bc: 8276036: The value of full_count in the message of insufficient codecache is wrong
8731846: 8276556: ProblemList java/nio/channels/FileChannel/LargeGatheringWrite.java on windows-x64
be1ca2b: 8276298: G1: Remove unused G1SegmentedArrayBufferList::add
... and 40 more: https://git.openjdk.java.net/jdk/compare/cef9db9a69061d51630e40b94ceba4b4bf03a0ce...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.

iklam

LGTM

yminqi · 2021-11-03T23:15:05Z

@iklam @calvinccheung Thanks for review!

yminqi · 2021-11-03T23:15:22Z

/integrate

openjdk · 2021-11-03T23:16:27Z

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

ce8c767: 8276220: Reduce excessive allocations in DateTimeFormatter
0ab910d: 8276066: Reset LoopPercentProfileLimit for x86 due to suboptimal performance
f3320d2: 8276588: Change "ccc" to "CSR" in HotSpot sources
32895ac: 8275650: Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11
c7f070f: 8276208: vmTestbase/nsk/jdb/repeat/repeat001/repeat001.java fails with "AssertionError: Unexpected output"
684edbb: 8273922: (fs) UserDefinedFileAttributeView doesn't handle file names that are just under the MAX_PATH limit (win)
7115892: 8276401: Use blessed modifier order in java.net.http
0ef8cbe: 8276385: Re-run blessed-modifier-order script on java.desktop and jdk.accessibility
724bf3b: 8275604: Zero: Reformat opclabels_data
61cb4bc: 8276036: The value of full_count in the message of insufficient codecache is wrong
... and 42 more: https://git.openjdk.java.net/jdk/compare/cef9db9a69061d51630e40b94ceba4b4bf03a0ce...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-11-03T23:16:46Z

@yminqi Pushed as commit 603bba2.

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

8271420: CDS should support custom loaders on all platforms

8c1e859

openjdk bot added hotspot core-libs labels Nov 2, 2021

openjdk bot removed the core-libs label Nov 2, 2021

openjdk bot removed the hotspot label Nov 2, 2021

openjdk bot added the hotspot-runtime label Nov 2, 2021

Add/Remove conversion between int/size_t for Windows

9460caf

yminqi changed the title ~~8271420: CDS should support custom loaders on all platforms~~ 8271420: Extend CDS custom loader support to Windows platform Nov 2, 2021

yminqi marked this pull request as ready for review November 2, 2021 15:42

openjdk bot added the rfr label Nov 2, 2021

iklam suggested changes Nov 2, 2021

View reviewed changes

calvinccheung reviewed Nov 2, 2021

View reviewed changes

calvinccheung suggested changes Nov 2, 2021

View reviewed changes

Revert change for not-custom loader related, fixed file relative path

66f7c4d

calvinccheung approved these changes Nov 3, 2021

View reviewed changes

openjdk bot added the ready label Nov 3, 2021

iklam approved these changes Nov 3, 2021

View reviewed changes

openjdk bot closed this Nov 3, 2021

openjdk bot added integrated and removed ready rfr labels Nov 3, 2021

yminqi deleted the jdk-8271420 branch December 20, 2021 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8271420: Extend CDS custom loader support to Windows platform #6202

8271420: Extend CDS custom loader support to Windows platform #6202

yminqi commented Nov 2, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 2, 2021

openjdk bot commented Nov 2, 2021

yminqi commented Nov 2, 2021

openjdk bot commented Nov 2, 2021

openjdk bot commented Nov 2, 2021

openjdk bot commented Nov 2, 2021

mlbridge bot commented Nov 2, 2021 •

edited

Loading

iklam left a comment

iklam Nov 2, 2021

iklam Nov 2, 2021

iklam Nov 2, 2021

iklam Nov 2, 2021

calvinccheung Nov 2, 2021

yminqi Nov 3, 2021

yminqi Nov 3, 2021

calvinccheung Nov 2, 2021

calvinccheung left a comment

openjdk bot commented Nov 3, 2021 •

edited

Loading

iklam left a comment

yminqi commented Nov 3, 2021

yminqi commented Nov 3, 2021

openjdk bot commented Nov 3, 2021

openjdk bot commented Nov 3, 2021

8271420: Extend CDS custom loader support to Windows platform #6202

8271420: Extend CDS custom loader support to Windows platform #6202

Conversation

yminqi commented Nov 2, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 2, 2021

openjdk bot commented Nov 2, 2021

yminqi commented Nov 2, 2021

openjdk bot commented Nov 2, 2021

openjdk bot commented Nov 2, 2021

openjdk bot commented Nov 2, 2021

mlbridge bot commented Nov 2, 2021 • edited Loading

Webrevs

iklam 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

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 Nov 3, 2021 • edited Loading

iklam left a comment

Choose a reason for hiding this comment

yminqi commented Nov 3, 2021

yminqi commented Nov 3, 2021

openjdk bot commented Nov 3, 2021

openjdk bot commented Nov 3, 2021

yminqi commented Nov 2, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 2, 2021 •

edited

Loading

openjdk bot commented Nov 3, 2021 •

edited

Loading