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

8280761: UseCompressedOops should be set after limit_heap_by_allocatable_memory #7938

Closed
wants to merge 7 commits into from

Conversation

tkiriyama
Copy link
Contributor

@tkiriyama tkiriyama commented Mar 24, 2022

I fixed to set UseCompressedOops flag after limit_heap_by_allocatable_memory().
So when ulimit -v is called and -XX:MaxRAM is set, UseCompressedOops does not become false.
And all hotspot tier1 test are passed.
Would you please review this fix?


Progress

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

Issue

  • JDK-8280761: UseCompressedOops should be set after limit_heap_by_allocatable_memory

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7938

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 24, 2022

👋 Welcome back tkiriyama! 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 changed the title 8280761: UseCompressedOops should be set after limit_heap_by_allocatable_memor 8280761: UseCompressedOops should be set after limit_heap_by_allocatable_memory Mar 24, 2022
@openjdk
Copy link

openjdk bot commented Mar 24, 2022

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

  • hotspot

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.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Mar 24, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 24, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 24, 2022

Webrevs

@tkiriyama
Copy link
Contributor Author

Could anyone review this pull request, please?

@dholmes-ora
Copy link
Member

/cc hotspot-gc

@openjdk openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Mar 29, 2022
@openjdk
Copy link

openjdk bot commented Mar 29, 2022

@dholmes-ora
The hotspot-gc label was successfully added.

@dholmes-ora
Copy link
Member

I cc'd hotspot-gc as they should probably be the ones to look at this for you.

@tkiriyama
Copy link
Contributor Author

Hello, I would appreciate it if anyone could review my fix.

@albertnetymk
Copy link
Member

I plan to take a look this week. Sorry for the delay; I was occupied by some other reviewing work last week.

@albertnetymk
Copy link
Member

I noticed that Github Action is not enabled for this PR. Please follow the instructions in https://wiki.openjdk.java.net/display/SKARA/Testing to enable Github Action. Also, I got build errors locally.

@tkiriyama
Copy link
Contributor Author

I noticed that Github Action is not enabled for this PR. Please follow the instructions in https://wiki.openjdk.java.net/display/SKARA/Testing to enable Github Action. Also, I got build errors locally.

I am very sorry. I enabled Github Action .
I have successfully built on Windows and Linux locally.

@albertnetymk
Copy link
Member

I enabled Github Action .

Doesn't look like GA is properly running. It should say sth like "12 successful checks", as shown in #7341.

I have successfully built on Windows and Linux locally.

OK, maybe this is a 32/64-bit thing. Having a working GA would have caught this.

@tschatzl
Copy link
Contributor

tschatzl commented Apr 6, 2022

The code will not compile on anything but 32 bit platforms. Here's a result on linux-x64:

.../src/hotspot/share/runtime/arguments.cpp: In static member function 'static void Arguments::set_heap_size()':
.../src/hotspot/share/runtime/arguments.cpp:1826:11: error: 'reasonable_max' was not declared in this scope
 1826 |       if (reasonable_max > max_coop_heap) {

reasonable_max used within the _LP64 block, and the change ends the scope it is declared in at line 1795. This also results in the entire block in the _LP64 block to be misaligned. I believe (without checking), the closing brace at 1795 has been intended to stay in line 1842.

Please fix.

Copy link
Member

@albertnetymk albertnetymk left a comment

Choose a reason for hiding this comment

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

The hotspot change looks good to me, but there's sth unclear to me in the test.

Comment on lines 84 to 86
// 1. Verify that MaxRAMPercentage overrides UseCompressedOops Ergo
// 2. Verify that UseCompressedOops forces compressed oops limit even
// when other flags are specified.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why MaxRAMPercentage or UseCompressedOops are relevant to this fix/bug.

args.add("-version");

String cmd = ProcessTools.getCommandLine(ProcessTools.createTestJvm(args.toArray(String[]::new)));
ProcessBuilder pb = new ProcessBuilder("sh", "-c", "ulimit -v 10485760;" + cmd);
Copy link
Member

Choose a reason for hiding this comment

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

The ulimit -v parameter should be passed in as well like MaxRAM. Then, from the calling context, one can see whether coop should be enabled or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review.

I don't understand why MaxRAMPercentage or UseCompressedOops are relevant to this fix/bug.

I forgot to remove this comment when I copied form TestMaxRAMFlags.java.
I fixed this comment.

The ulimit -v parameter should be passed in as well like MaxRAM. Then, from the calling context, one can see whether coop should be enabled or not.

This bug appears when ulimit -v is specified and MaxRAM is set bigger value. So I think it does not need that
ulimit -v parameter let be variable.

Copy link
Member

Choose a reason for hiding this comment

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

It's not about whether it's constant or variable.

In this context, it's unclear why coop is enabled.

// Args: MaxRAM , MaxRAMPercentage, forcecoop, expect coop
checkFlag(32 * oneG, 100, false, true);

checkFlag should not sneak in logic altering the coop value.

Copy link
Contributor

@tschatzl tschatzl left a comment

Choose a reason for hiding this comment

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

Looks okay.

@openjdk
Copy link

openjdk bot commented Apr 8, 2022

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

8280761: UseCompressedOops should be set after limit_heap_by_allocatable_memory

Reviewed-by: ayang, tschatzl

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

  • 21ea740: 8284699: Include all image types to the J2DBench.ColorConvertOpTests
  • e5041ae: 8144030: [macosx] test java/awt/Frame/ShapeNotSetSometimes/ShapeNotSetSometimes.java fails (again)
  • f5beafa: 8159599: [TEST_BUG] java/awt/Modal/ModalInternalFrameTest/ModalInternalFrameTest.java
  • 21de4e5: 8284681: compiler/c2/aarch64/TestFarJump.java fails with "RuntimeException: for CodeHeap < 250MB the far jump is expected to be encoded with a single branch instruction"
  • 9695283: 8240903: Add test to check that jmod hashes are reproducible
  • dce7240: 8284921: tier1 test failures after JDK-8284909
  • 9f97f5d: 8283704: Add sealed modifier to java.awt.MultipleGradientPaint
  • 1ebf2f0: 8284909: [JVMCI] remove remnants of AOT support
  • 6199008: 8284914: Problem list test(s) failing due to extra repaints with D3D pipeline.
  • 4cc8ecc: 8280915: Better parallelization for AbstractSpliterator and IteratorSpliterator when size is unknown
  • ... and 271 more: https://git.openjdk.java.net/jdk/compare/e6f707aa76ac231bef7d0abf1dd643bd7471067f...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@albertnetymk, @tschatzl) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 8, 2022
Copy link
Member

@albertnetymk albertnetymk left a comment

Choose a reason for hiding this comment

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

In summary, the signature I have in mind is sth like checkFlag(long ulimit_v, long max_ram, boolean expected_coop).

args.add("-version");

String cmd = ProcessTools.getCommandLine(ProcessTools.createTestJvm(args.toArray(String[]::new)));
ProcessBuilder pb = new ProcessBuilder("sh", "-c", "ulimit -v 10485760;" + cmd);
Copy link
Member

Choose a reason for hiding this comment

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

It's not about whether it's constant or variable.

In this context, it's unclear why coop is enabled.

// Args: MaxRAM , MaxRAMPercentage, forcecoop, expect coop
checkFlag(32 * oneG, 100, false, true);

checkFlag should not sneak in logic altering the coop value.

Comment on lines 85 to 86
// 2. Verify that UseCompressedOops forces compressed oops limit even
// when ulimit -v are specified.
Copy link
Member

Choose a reason for hiding this comment

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

I understand the first bullet but not the second. Looking at the reproducers in the JBS ticket, UseCompressedOops should not be provided; it's read only.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like an additional (unrelated) test to make sure it is overridden if necessary imho. It seems a bit forced though because there is no test that actually verifies that UseCompressedOops is false later.
I'm good with removing the test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like an additional (unrelated) test to make sure it is overridden if necessary imho.

That is correct.
Do you mean that the following code should be removed?

checkFlag(128 * oneG, 100, true, true);

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Removing this test case would limit the testing to the actual problem stated in the CR. Verifying that UseCompressedOops is enabled correctly is tested somewhere else already, and there is no reason that using ulimit -v would change that behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand.
I removed this test patern.


long oneG = 1L * 1024L * 1024L * 1024L;

// Args: MaxRAM , MaxRAMPercentage, forcecoop, expect coop
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why MaxRAMPercentage is needed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is that the initial value for reasonable_max is exactly MaxRAM; you are right that for this test it is not necessary to pass it explicitly, just always set it to 100 percent in the command line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider that values like MaxRAM might change in the future (unlikely as it is). Setting it makes following the execution a bit easier imho.

Copy link
Member

Choose a reason for hiding this comment

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

Aha, I see. Having MaxRAMPercentage set explicitly instead of relying on its default value indeed makes the test more resilient. Please add a comment for MaxRAMPercentage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.
I added a comment for MaxRAMPercentage.

long oneG = 1L * 1024L * 1024L * 1024L;

// Args: MaxRAM , MaxRAMPercentage, expect coop
// Having MaxRAMPercentage set explicitly instead of relying on its default value indeed makes the test more resilient.
Copy link
Member

Choose a reason for hiding this comment

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

Slight rewording, // Setting MaxRAMPercentage explicitly to make the test more resilient.

Copy link
Member

@albertnetymk albertnetymk left a comment

Choose a reason for hiding this comment

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

Thank you for the revision. Embedding ulimit -v inside checkFlag still hinders readability, IMO.

What I have in mind is:

// ulimit, max_ram, max_ram_percent, expected_coop
checkFlag(5 * oneG, 32 * oneG, 100, true);

Then, it's clear in this context why coop is enabled.

@tkiriyama
Copy link
Contributor Author

tkiriyama commented Apr 13, 2022

Slight rewording, // Setting MaxRAMPercentage explicitly to make the test more resilient.

Thank you, I fixed this comment.

Embedding ulimit -v inside checkFlag still hinders readability, IMO.

Sounds good. I fixed this test as you advised.

@albertnetymk
Copy link
Member

Need sth like this to resolve the failure.

  // Convert bytes to kbytes for ulimit -v
  var ulimit_prefix = "ulimit -v " + (ulimit / 1024);
  ...
  ProcessBuilder pb = new ProcessBuilder("sh", "-c", ulimit_prefix + ";" + cmd);

@tkiriyama
Copy link
Contributor Author

Need sth like this to resolve the failure.

I'm sorry. This is my mistake. I added your code.

@tkiriyama
Copy link
Contributor Author

I appreciate all reviews. I hope this change is integrated.

@tkiriyama
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 18, 2022
@openjdk
Copy link

openjdk bot commented Apr 18, 2022

@tkiriyama
Your change (at version 65223a5) is now ready to be sponsored by a Committer.

@albertnetymk
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Apr 19, 2022

Going to push as commit 8d96ab0.
Since your change was applied there have been 295 commits pushed to the master branch:

  • b9f513c: 8283790: G1: Remove redundant card/heap-address transition
  • 647aa2a: 8284572: Remove unneeded null check in ReferenceProcessor::discover_reference
  • ab83bce: 8284661: Reproducible assembly builds without relative linking
  • c5e9719: 8266246: Swing test PressedIconTest.java sometimes fails on macOS 11 ARM
  • 447c2d1: 8284521: Write an automated regression test for RFE 4371575
  • 145dfed: 8284937: riscv: should not allocate special register for temp
  • 87faa85: 8186958: Need method to create pre-sized HashMap
  • 41fc078: 8284112: Minor cleanup could be done in javax.crypto
  • 897d6c0: 8282008: Incorrect handling of quoted arguments in ProcessBuilder
  • ffdeb32: 8284928: Add links from SourceVersion to specific JLS versions
  • ... and 285 more: https://git.openjdk.java.net/jdk/compare/e6f707aa76ac231bef7d0abf1dd643bd7471067f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Apr 19, 2022
@openjdk openjdk bot closed this Apr 19, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 19, 2022
@openjdk
Copy link

openjdk bot commented Apr 19, 2022

@albertnetymk @tkiriyama Pushed as commit 8d96ab0.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org hotspot-gc hotspot-gc-dev@openjdk.org integrated Pull request has been integrated
4 participants