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

[David Holmes]

Chris,

The suggested patch removes the ability to set MaxDirectMemorySize to 
zero, thereby preventing use of direct memory completely. I don't think 
this has any practical significance (a value of 1 will disable it just 
as effectively), but it does impact a NIO test and so I want to run this 
past the NIO folks - now cc'ed.

NIO folk: the main issue was that use of intx prevented setting this 
 >2GB on 32-bit. So the proposed change is to use uintx, which means we 
can no longer use -1 as the "use the default" setting. Zero was chosen 
instead, but that now prevents setting an actual limit of zero. Note the 
VM still sets the property to -1 when it sees zero, so the JDK side of 
this is unchanged. Perhaps the VM could use "uintx_max" instead?

There is also an issue of whether this needs to go through CCC and if we 
need to document this in the release notes (assuming it goes ahead). 
This is not a supported option and there is nothing I can find that 
documents the "use the default" setting, other than the source code.

Aside: in the process I discovered that the test 
java/nio/Buffer/LimitDirectMemory.sh actually assumes that -1 is an 
illegal value. But the script is written incorrectly and so fails for 
the wrong reason (and the fail equates to success). I've filed 7180649 
for this.

Thanks,
David

On 29/06/2012 7:05 AM, David Holmes wrote:
> <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
>>>>>
>>
>>