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

8260349: Cannot programmatically retrieve Metaspace max set via JAVA_TOOL_OPTIONS #2275

Closed
wants to merge 9 commits into from

Conversation

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Jan 28, 2021

A simple but long standing bug whereby the MemoryPool MXBean only sees the value of MaxMetaspaceSize if it was set directly on the command-line and not by other means (e.g. env var).

Fix is to check !FLAG_IS_DEFAULT rather than FLAG_IS_CMDLINE.

Expanded regression test added (based on the reproducer in the JBS issue) that checks all the ways to set the flag: cmd-line, env-var, hotspotrc file

Testing: regression test (before and after fix)
tiers 1-3 (in progress)

Thanks,
David


Progress

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

Issue

  • JDK-8260349: Cannot programmatically retrieve Metaspace max set via JAVA_TOOL_OPTIONS

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2275/head:pull/2275
$ git checkout pull/2275

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 28, 2021

👋 Welcome back dholmes! 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.

@dholmes-ora dholmes-ora marked this pull request as ready for review Jan 28, 2021
@openjdk openjdk bot added the rfr label Jan 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 28, 2021

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

@dholmes-ora dholmes-ora changed the title 8260349: Cannot programmatically retrieve Metaspace max set via JAVA_TOOLS_OPTIONS 8260349: Cannot programmatically retrieve Metaspace max set via JAVA_TOOL_OPTIONS Jan 28, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 28, 2021

Copy link
Contributor

@shipilev shipilev left a comment

The fix looks right. I suggest polishing the test a little.

import java.io.PrintWriter;
import java.lang.management.ManagementFactory;
import java.lang.management.MemoryPoolMXBean;

import jdk.test.lib.process.ProcessTools;
import jdk.test.lib.process.OutputAnalyzer;

/*
* @test
* @bug 8260349
* @summary test that setting via the env-var and options file shows up as expected
* @library /test/lib
* @run driver MaxMetaspaceSizeEnvVarTest
*/
Copy link
Contributor

@shipilev shipilev Jan 28, 2021

I believe the conventional style is to do jtreg test definition the first thing, and then do imports.

Copy link
Member Author

@dholmes-ora dholmes-ora Jan 28, 2021

Thanks for the Review Aleksey - note I just updated the test with a missing test case.

I'm not aware of a specific convention here. :) I used the existing MaxMetaspaceSizeTest.java in the same directory as the template for this one. But I can change it if you insist.

Thanks,
David

Copy link
Contributor

@shipilev shipilev Jan 28, 2021

Please change if you are doing the test touchups anyway.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 28, 2021

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

8260349: Cannot programmatically retrieve Metaspace max set via JAVA_TOOL_OPTIONS

Reviewed-by: shade, stuefe

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.

@openjdk openjdk bot added the ready label Jan 28, 2021
Copy link
Contributor

@shipilev shipilev left a comment

Good. Minor nits to fix before push?

Copy link
Member

@tstuefe tstuefe left a comment

Hi David,

this looks fine to me. Some nits below, but feel free to ignore them. Thanks for fixing this.

..Thomas

return FLAG_IS_CMDLINE(MaxMetaspaceSize) ? MaxMetaspaceSize :
MemoryUsage::undefined_size();
return !FLAG_IS_DEFAULT(MaxMetaspaceSize) ? MaxMetaspaceSize :
MemoryUsage::undefined_size();
Copy link
Member

@tstuefe tstuefe Jan 28, 2021

Somewhat more robust may be to compare against max_uint. In case we ever change the default to not be infinite. But I am fine with this too.

}

public static void main(String... args) throws Exception {
final String max = String.valueOf(9 * 1024 * 1024); // 9 MB
Copy link
Member

@tstuefe tstuefe Jan 28, 2021

You could set this to a much larger value for this test, since it won't change how the test works and would not cause more footprint. Does not matter here but may matter if we ever backport this, since before jep387 initial metaspace usage was a lot higher (400K vs 5-6m).

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 28, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Thomas,

Thanks for taking a look at this.

On 28/01/2021 9:33 pm, Thomas Stuefe wrote:

On Thu, 28 Jan 2021 10:53:02 GMT, David Holmes <dholmes at openjdk.org> wrote:

A simple but long standing bug whereby the MemoryPool MXBean only sees the value of MaxMetaspaceSize if it was set directly on the command-line and not by other means (e.g. env var).

Fix is to check !FLAG_IS_DEFAULT rather than FLAG_IS_CMDLINE.

Expanded regression test added (based on the reproducer in the JBS issue) that checks all the ways to set the flag: cmd-line, env-var, hotspotrc file

Testing: regression test (before and after fix)
tiers 1-3 (in progress)

Thanks,
David

David Holmes has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:

- Further formatting updates from Aleksey
- Merge branch 'master' into 8260349
- Cleaned up test formatting based on Aleksey's review
- Added missing test case for flag not set
- Boost max from 1MB to 9MB as 1MB was too low for Aarch64 to startup
- Reduced test code using loop for env-vars
- 8260349: Cannot programmatically retrieve Metaspace max set via JAVA_TOOLS_OPTIONS

Hi David,

this looks fine to me. Some nits below, but feel free to ignore them. Thanks for fixing this.

..Thomas

src/hotspot/share/services/memoryPool.cpp line 201:

199: size_t MetaspacePool::calculate_max_size() const {
200: return !FLAG_IS_DEFAULT(MaxMetaspaceSize) ? MaxMetaspaceSize :
201: MemoryUsage::undefined_size();

Somewhat more robust may be to compare against max_uint. In case we ever change the default to not be infinite. But I am fine with this too.

I'll leave it as is.

test/hotspot/jtreg/runtime/Metaspace/MaxMetaspaceSizeEnvVarTest.java line 66:

64:
65: public static void main(String... args) throws Exception {
66: final String max = String.valueOf(9 * 1024 * 1024); // 9 MB

You could set this to a much larger value for this test, since it won't change how the test works and would not cause more footprint. Does not matter here but may matter if we ever backport this, since before jep387 initial metaspace usage was a lot higher (400K vs 5-6m).

Actually the 9MB was selected because I found that on earlier versions
it was a lot higher. 8M failed and 9M passed.

Thanks again.
David
-----

@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented Feb 1, 2021

/integrate

@openjdk openjdk bot closed this Feb 1, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 1, 2021
@dholmes-ora dholmes-ora deleted the 8260349 branch Feb 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 1, 2021

@dholmes-ora Pushed as commit b6a7367.

💡 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
3 participants