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

8271419: Refactor test code for modifying CDS archive contents #4945

Closed
wants to merge 4 commits into from

Conversation

yminqi
Copy link
Contributor

@yminqi yminqi commented Jul 30, 2021

The code for modifying CDS archive content could be used by other tests so the common code is refactored into a util class CDSArchiveUtils.

Tests: tier1,tier4

Thanks
Yumin


Progress

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

Issue

  • JDK-8271419: Refactor test code for modifying CDS archive contents

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4945

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 30, 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.

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Jul 30, 2021

/label add hotspot-runtime

@openjdk openjdk bot added rfr hotspot-runtime labels Jul 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 30, 2021

@yminqi
The hotspot-runtime label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 30, 2021

Webrevs

alignment = wb.metaspaceSharedRegionAlignment();
// fileHeaderSize may not be available
// fileHeaderSize = (int)alignUpWithPageSize(wb.getOffsetForName("file_header_size"));
}
Copy link
Member

@iklam iklam Jul 30, 2021

Choose a reason for hiding this comment

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

This should always be available. file_header_size is a constant value (sizeof(FileMapHeader)) stored inside cdsoffsets.cpp. It doesn't depend on the contents of the archive file.

Copy link
Contributor Author

@yminqi yminqi Jul 30, 2021

Choose a reason for hiding this comment

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

This is same as original code, I will investigate more of this part. Thanks.

if (fc.isOpen()) {
fc.close();
}
System.out.println(" offset_jvm_ident " + CDSArchiveUtils.offsetJvmIdent);
Copy link
Member

@iklam iklam Jul 30, 2021

Choose a reason for hiding this comment

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

I think the modifyXXX functions should also be moved to CDSArchiveUtils so that they can be called by other test cases.

Copy link
Contributor Author

@yminqi yminqi Jul 30, 2021

Choose a reason for hiding this comment

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

Let me double check this part.

if (fc.isOpen()) {
fc.close();
}
byte[] buf = { (byte)(value >> 24), (byte)(value >> 16), (byte)(value >> 8), (byte)(value)};
Copy link
Member

@calvinccheung calvinccheung Jul 30, 2021

Choose a reason for hiding this comment

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

Isn't this the same as the original code which does a ByteBuffer.wrap()?
If so, why changing it.

Copy link
Contributor Author

@yminqi yminqi Jul 30, 2021

Choose a reason for hiding this comment

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

I make CDSARchiveUtils take byte[] as input, not ByteBuffer. Here just simply convert int to byte[].

WhiteBox box = WhiteBox.getWhiteBox();
CDSArchiveUtils.initialize(box); // all offsets available
Copy link
Member

@calvinccheung calvinccheung Jul 30, 2021

Choose a reason for hiding this comment

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

After this refactoring, I don't think WhiteBox is required in this file.
So I think CDSArchiveUtils could create a WhiteBox instance instead of requiring the caller to supply one.

Copy link
Contributor Author

@yminqi yminqi Jul 30, 2021

Choose a reason for hiding this comment

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

The util class is just a utility class, I have no idea if we can do this, let me try --- it might work.

Copy link
Member

@calvinccheung calvinccheung left a comment

I have 2 comments.

@@ -89,261 +62,21 @@
public static int num_regions = shared_region_name.length;
public static String[] matchMessages = {
"Unable to use shared archive",
"The shared archive file has a bad magic number",
"Unable to map shared spaces",
Copy link
Member

@iklam iklam Aug 2, 2021

Choose a reason for hiding this comment

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

Why are these new errors needed? Did you change any behavior of the tests?

Copy link
Contributor Author

@yminqi yminqi Aug 2, 2021

Choose a reason for hiding this comment

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

Thanks for mentioning this. There is an issue to use FileChannel:
outputChannel.transferFrom(inputChannel, 0, offset);
+ outputChannel.position(offset); // <---- need explicitly positioned or the following write to overwrite from offset 0.
outputChannel.write(ByteBuffer.wrap(bytes));
outputChannel.transferFrom(inputChannel, offset + bytes.length, orgSize - bytes.length);

The added lines should be removed.

public static int num_regions = shared_region_name.length;

public static void initialize() throws Exception {
WhiteBox wb = WhiteBox.getWhiteBox();
Copy link
Member

@iklam iklam Aug 2, 2021

Choose a reason for hiding this comment

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

This method can be changed to a static { ....} block, so that you don't need to explicitly call CDSArchiveUtils.initialize().

Copy link
Contributor Author

@yminqi yminqi Aug 2, 2021

Choose a reason for hiding this comment

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

Will move to static block.

@@ -34,42 +34,15 @@
* @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI SharedArchiveConsistency on
* @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI SharedArchiveConsistency auto
*/
import jdk.test.lib.cds.CDSArchiveUtils;
import jdk.test.lib.process.OutputAnalyzer;
import jdk.test.lib.Utils;
Copy link
Member

@calvinccheung calvinccheung Aug 2, 2021

Choose a reason for hiding this comment

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

The above import of Utils is no longer needed.

Copy link
Contributor Author

@yminqi yminqi Aug 2, 2021

Choose a reason for hiding this comment

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

This one is new added util class, it is needed here. Do you think it is CDSTestUtils ?

Copy link
Member

@calvinccheung calvinccheung Aug 2, 2021

Choose a reason for hiding this comment

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

I was referring to import jdk.test.lib.Utils;. It was for Utils.getRandomInstance(); and the code has been moved to the new class CDSArchiveUtils.

Copy link
Contributor Author

@yminqi yminqi Aug 2, 2021

Choose a reason for hiding this comment

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

I see, thanks. Will remove.

import java.util.Random;
import sun.hotspot.WhiteBox;
Copy link
Member

@calvinccheung calvinccheung Aug 2, 2021

Choose a reason for hiding this comment

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

The import of Random is no longer needed.
I think the import of WhiteBox can also be removed since it is not used in this file.

Copy link
Contributor Author

@yminqi yminqi Aug 2, 2021

Choose a reason for hiding this comment

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

Yes, they no longer needed. thanks.

Copy link
Member

@calvinccheung calvinccheung left a comment

Just couple of nits.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 2, 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:

8271419: Refactor test code for modifying CDS archive contents

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

  • 0a85236: 8193559: ugly DO_JAVA_THREADS macro should be replaced
  • db950ca: 8271348: Add stronger sanity check of thread state when polling for safepoint/handshakes
  • 3e3051e: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside
  • 7a4c754: 8271611: Use SecurityConstants.ACCESS_PERMISSION in MethodHandles
  • e74537f: 8271605: Update JMH devkit to 1.32
  • 249d641: 8263561: Re-examine uses of LinkedList
  • 6a3f834: 8268113: Re-use Long.hashCode() where possible
  • 2536e43: 8270160: Remove redundant bounds check from AbstractStringBuilder.charAt()
  • 6c4c48f: 8266972: Use String.concat() in j.l.Class where invokedynamic-based String concatenation is not available
  • 72145f3: 8269665: Clean-up toString() methods of some primitive wrappers
  • ... and 18 more: https://git.openjdk.java.net/jdk/compare/d09b028407ff9d0e8c2dfd9cc5d0dca19c4497e3...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 Aug 2, 2021
iklam
iklam approved these changes Aug 2, 2021
Copy link
Member

@iklam iklam left a comment

LGTM

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Aug 2, 2021

@calvinccheung @iklam Thanks for review!
/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Aug 2, 2021

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

  • 0b95394: 8271624: Avoid unnecessary ThreadGroup.checkAccess calls when creating Threads
  • e621cff: 8271627: Use local field access in favor of Class.getClassLoader0
  • 0a85236: 8193559: ugly DO_JAVA_THREADS macro should be replaced
  • db950ca: 8271348: Add stronger sanity check of thread state when polling for safepoint/handshakes
  • 3e3051e: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside
  • 7a4c754: 8271611: Use SecurityConstants.ACCESS_PERMISSION in MethodHandles
  • e74537f: 8271605: Update JMH devkit to 1.32
  • 249d641: 8263561: Re-examine uses of LinkedList
  • 6a3f834: 8268113: Re-use Long.hashCode() where possible
  • 2536e43: 8270160: Remove redundant bounds check from AbstractStringBuilder.charAt()
  • ... and 20 more: https://git.openjdk.java.net/jdk/compare/d09b028407ff9d0e8c2dfd9cc5d0dca19c4497e3...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Aug 2, 2021

@yminqi Pushed as commit 84f0231.

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

@yminqi yminqi deleted the jdk-8271419 branch Aug 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