RFR: 8260349: Cannot programmatically retrieve Metaspace max set via JAVA_TOOL_OPTIONS [v4]

[David Holmes]

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

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