RFR 7179383 (was Re: -XX:MaxDirectMemorySize argument parsing)

[David Holmes]

<Bengt's original response was sent internally>

Hi Bengt,

Thanks for taking a detailed look at this contribution. I now have a
large group of reviewers :)

On 28/06/2012 11:08 PM, Bengt Rutisson wrote:
> I have a question regarding this change. When I look at
> VM.saveAndRemoveProperties(), which parses the
> "sun.nio.MaxDirectMemorySize" option that hotspot sets, it looks to me
> like it is intended to have three cases:
>
> (1) option not set - use default value of 64 MB
> (2) option is set to "-1" - use all available memory for native allocation
> (3) option is set to "an actual value" - use the value as a limit for
> native allocation
>
> See:
> http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/b92353a01aa0/src/share/classes/sun/misc/VM.java
>
>
> So, it seems to me that before your change it would be possible for a
> user to set -XX:MaxDirectMemorySize=-1 to make the JDK use all available
> memory for native allocations. But after your change it will not be
> possible set this value.

With the change the user will set -XX:MaxDirectMemorySize=0 on the
command line, which is then used to set the property to -1 and so
instruct use of all available memory. So the only difference in that
regard is what value gets specified on the command-line to get the
default behaviour of "all available memory".

> On the other hand hotspot always sets the "sun.nio.MaxDirectMemorySize"
> property, which means that case (1) above can't really happen. To me
> this is strange and I sent a question to Alan Bateman and Chris Hegarty
> about that. This means that there is really no need for a user to have
> -XX:MaxDirectMemorySize=-1 on the command line.

I agree there is no need for the user to ever have to specify -1 as that 
is the current default. Case (1) is the 64MB "default" but it isn't
really a default. The comments before it state:

// The initial value of this field is arbitrary; during JRE initialization
// it will be reset to the value specified on the command line, if any,
// otherwise to Runtime.getRuntime().maxMemory().
//
private static long directMemory = 64 * 1024 * 1024;

So there are only two expected cases as hotspot always sets the
property: either to -1 (which says use all available memory), else the
set value.

The 64MB will only be used if either the property is not set (doesn't
happen for hotspot) or else it was set to a negative value < -1. The
change prevents that second error case from occurring.

However I have just realized that there is now no way to set the value
to zero. Not that this is useful (no small value is at all
useful/practical) but there may be a test somewhere that checks boundary 
values.

> I think your change is fine since it was already impossible to get to
> (1) and thus the default was in practice (2), which will also be the
> default after your change. Hopefully there are not many customers with
> -XX:MaxDirectMemorySize=-1 on their command line.

I was concerned about this too. But I could not find anything documented 
about the value -1 having special meaning, so no reason for anyone to 
use that. It would also be redundant as that is the internal default set 
by the VM. Anyone using that will now get an error and realize something 
has changed. Which would suggest we may need something in the release notes.

> So, to conclude, I think your change is fine, but it looks to me like
> VM.java and jvm.cpp are not really in agreement of what the default
> value for the maximum direct buffer limit should be.

Thanks again for the detailed review.

David
-----

> Bengt
>
>
> On 2012-06-28 07:42, David Holmes wrote:
>> Can I please get someone else to take a look at this community
>> contribution - _pretty pleeze_ ;-)
>>
>> Thanks,
>> David
>>
>> On 25/06/2012 3:20 PM, David Holmes wrote:
>>> Sorry for the delay on this Chris. I've filed 7179383 and generated a
>>> webrev:
>>>
>>> http://cr.openjdk.java.net/~dholmes/7179383/webrev/
>>>
>>> Still need an additional runtime reviewer.
>>>
>>> Thanks,
>>> David
>>>
>>> On 11/06/2012 11:18 PM, Chris Dennis wrote:
>>>> On Jun 7, 2012, at 9:41 PM, David Holmes wrote:
>>>>
>>>>> On 8/06/2012 12:20 AM, Alan Bateman wrote:
>>>>>> On 07/06/2012 14:28, Chris Dennis wrote:
>>>>>>> Yes, I'm listed under "Terracotta Inc. (Christopher Dennis)".
>>>>>>>
>>>>>>> There is one additional complication to this in that the
>>>>>>> LimitDirectMemory test in the jdk sources is currently broken. The
>>>>>>> patch below "fixes" the test - but leaves two open questions:
>>>>>>>
>>>>>>> What should the grep be looking for? This is JDK test asserting on
>>>>>>> output generated by Hotspot - that seems a little screwed up to me,
>>>>>>> right?
>>>>>>>
>>>>>>> Chris
>>>>>> it might be nicer to just check the exit code and not depend on the
>>>>>> error message.
>>>>>
>>>>> Agreed. Seems cleaner.
>>>> Okay, I'll prepare a second jdk patch that modifies this test to use
>>>> the exit value of the JVM as the indicator of startup failure. Once we
>>>> have a bug-id for this issue I'll propose the test patch on the
>>>> relevant mailing list referencing the upcoming behavior change and our
>>>> desire for a more hotspot-neutral test assertion.
>>>>
>>>> Alan: What would the correct forest to provide and patch against and
>>>> which mailing list should I post it to?
>>>>
>>>>>
>>>>>> Just on logistics, as hotspot and jdk changes take a
>>>>>> different route into master it means that we'll need to wait until
>>>>>> the
>>>>>> hotspot changes get to jdk8/jdk8 (and probably down to jdk8/tl)
>>>>>> before
>>>>>> pushing a change to the LimitDirectMemory.sh test.
>>>>>
>>>>> Also is the hotspot fix targeted for 8 and 7u, or just 8?
>>>> I'm not sure if this question was intended for me, but as far as I'm
>>>> aware currently this change doesn't even have a bug-id. Personally, I
>>>> don't see the pressing need to have it merged back to 7u, and not
>>>> doing so would help mitigate the backwards-compatibility issue of the
>>>> subtle changes it makes in the behavior of the switch.
>>>>
>>>>
>>>>>
>>>>> Still need additional reviewer from runtime - thanks.
>>>>>
>>>>> David
>>>>>
>>>>>> -Alan
>>>>
>
>