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

David Holmes david.holmes at oracle.com
Thu Jun 28 22:44:10 PDT 2012 

Hi Chris,

On 29/06/2012 1:27 PM, Chris Dennis wrote:
> Hi All,
>
> I'm not sure this analysis is correct.  If I understand how this all works correctly then the default value specified for MaxDirectMemorySize in globals.hpp is really not important.  The command line parsing understands both the origin of a flag and it's value.  "FLAG_IS_DEFAULT(MaxDirectMemorySize)" doesn't translate to "is the current value of the MaxDirectMemorySize flag the same as it's default value" but instead to "is the origin of the current value of the MaxDirectMemorySize flag not the default" (the other options being command-line or ergonomic).  I ran the following test class:

You are right, I was misinterpreting what FLAG_IS_DEFAULT meant. So all 
is well with the fix and we have a plethora of reviewers.

I'd also forgotten about the earlier discussion regarding the test, but 
at least now there is a CR for it. :)

I'm just doing a fresh test build as it turned out my testing the other 
day didn't include the fix. I'm hoping the bugs in the test program mean 
the new functionality will still "pass" on the broken test.

Thanks,
David
-----

> public class TestMaxDirectMemory {
>      public static void main(String[] args) {
>          System.out.println(sun.misc.VM.maxDirectMemory());
>      }
> }
>
> on a JVM with and without my changes:
>
> java -Xmx128m TestMaxDirectMemory
> 	current:	119341056
> 	patched:	119341056
> java -XX:MaxDirectMemorySize=-2 -Xmx128m TestMaxDirectMemory
> 	current:	67108864
> 	patched:	Invalid maximum direct memory size: -XX:MaxDirectMemorySize=-2
> java -XX:MaxDirectMemorySize=-1 -Xmx128m TestMaxDirectMemory
> 	current:	119341056
> 	patched:	Invalid maximum direct memory size: -XX:MaxDirectMemorySize=-1
> java -XX:MaxDirectMemorySize=0 -Xmx128m TestMaxDirectMemory
> 	current:	0
> 	patched:	0
> java -XX:MaxDirectMemorySize=1 -Xmx128m TestMaxDirectMemory
> 	current:	1
> 	patched:	1
> java -XX:MaxDirectMemorySize=96m -Xmx128m TestMaxDirectMemory
> 	current:	100663296
> 	patched:	100663296
>
> Everything seems to be working fine as far as I can tell.  The only changes in behavior that might be concerning for someone here are the changes for negative values, you can still specify zero if you so wish (although as David points out this would be a pretty strange thing to do).
>
> As for the broken test, back when we originally discussed it I think Alan Bateman suggested that we change the test for this to assert not on the textual output of the failing process but instead on the return value (since a JDK test asserting on Hotspot generated output seemed wrong to everyone involved).  I actually have the changes to fix the test prepped and ready to submit but I imagine it will be simpler for us to wait until this patch has been integrated in to the JDK8 forest before submitting the test fix to avoid causing confusion (or test failures) in the JDK8 forest if the fix were to get merged too soon.
>
> Thanks,
>
> Chris
>
> On Jun 28, 2012, at 9:24 PM, David Holmes wrote:
>
>> 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
>>>>>>>
>>>>
>>>>
>

More information about the hotspot-runtime-dev mailing list