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

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

Closed
wants to merge 3 commits into from

Conversation

yminqi
Copy link
Contributor

@yminqi yminqi commented Nov 2, 2021

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

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
Copy link

@bridgekeeper bridgekeeper bot commented Nov 2, 2021

👋 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
Copy link

@openjdk openjdk bot commented Nov 2, 2021

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

@openjdk openjdk bot added hotspot core-libs labels Nov 2, 2021
@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Nov 2, 2021

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

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

@openjdk openjdk bot commented Nov 2, 2021

@yminqi
The core-libs label was successfully removed.

@openjdk openjdk bot removed the hotspot label Nov 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 2, 2021

@yminqi
The hotspot label was successfully removed.

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

@openjdk openjdk bot commented Nov 2, 2021

@yminqi
The hotspot-runtime label was successfully added.

@yminqi 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 yminqi marked this pull request as ready for review Nov 2, 2021
@openjdk openjdk bot added the rfr label Nov 2, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 2, 2021

Webrevs

Copy link
Member

@iklam iklam left a comment

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.

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

int len = obj->size();
int len = (int)obj->size();
Copy link
Member

@iklam iklam Nov 2, 2021

Choose a reason for hiding this comment

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

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

@@ -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)))
Copy link
Member

@iklam iklam Nov 2, 2021

Choose a reason for hiding this comment

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

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

@@ -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)
Copy link
Member

@iklam iklam Nov 2, 2021

Choose a reason for hiding this comment

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

This change is unrelated to custom class loaders.

@@ -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());
Copy link
Member

@iklam iklam Nov 2, 2021

Choose a reason for hiding this comment

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

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

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

int len = obj->size();
int len = (int)obj->size();
Copy link
Member

@calvinccheung calvinccheung Nov 2, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@yminqi yminqi Nov 3, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@yminqi yminqi Nov 3, 2021

Choose a reason for hiding this comment

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

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

@@ -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()};
Copy link
Member

@calvinccheung calvinccheung Nov 2, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@calvinccheung calvinccheung left a comment

I have couple of comments.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2021

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

@openjdk openjdk bot added the ready label Nov 3, 2021
iklam
iklam approved these changes Nov 3, 2021
Copy link
Member

@iklam iklam left a comment

LGTM

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Nov 3, 2021

@iklam @calvinccheung Thanks for review!

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Nov 3, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2021

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 openjdk bot closed this Nov 3, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2021

@yminqi Pushed as commit 603bba2.

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

@yminqi yminqi deleted the jdk-8271420 branch Dec 20, 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