[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8260349: Cannot programmatically retrieve Metaspace max set via JAVA_TOOL_OPTIONS [v4]
From:       David Holmes <david.holmes () oracle ! com>
Date:       2021-01-28 13:14:35
Message-ID: 9f9ca94c-597d-9c4a-dbff-78c43f6fc633 () oracle ! com
[Download RAW message or body]

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

> -------------
> 
> Marked as reviewed by stuefe (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/2275
> 


[prev in list] [next in list] [prev in thread] [next in thread]