RFR (S) 8227123: Assertion failure when setting SymbolTableSize larger than 2^17 (131,072)

[David Holmes]

On 24/07/2019 1:48 am, coleen.phillimore at oracle.com wrote:
> On 7/23/19 11:30 AM, Daniel D. Daugherty wrote:
>> On 7/23/19 11:09 AM, coleen.phillimore at oracle.com wrote:
>>> On 7/23/19 9:45 AM, Daniel D. Daugherty wrote:
>>>> On 7/23/19 7:03 AM, coleen.phillimore at oracle.com wrote:
>>>>> On 7/23/19 12:27 AM, David Holmes wrote:
>>>>>> Hi Coleen,
>>>>>>
>>>>>> -  experimental(bool, UnlockExperimentalVMOptions, false, \
>>>>>> +  experimental(bool, UnlockExperimentalVMOptions, trueInDebug,     \
>>>>>>
>>>>>> I can't quite convince myself this is harmless nor necessary.
>>>>>
>>>>> Well if it's added, then the option range test would test the 
>>>>> option.  Otherwise, I think it's benign.  In debug mode, one would 
>>>>> no longer have to specify -XX:+UnlockExperimental options, just 
>>>>> like UnlockDiagnosticVMOptions.   The option is there either way.
>>>>
>>>> Mentioning 'UnlockDiagnosticVMOptions' reminds me that some folks think
>>>> that 'UnlockDiagnosticVMOptions' being 'trueInDebug' can cause bugs 
>>>> in tests
>>>> that are runnable in all build configs: 'release', 'fastdebug' and 
>>>> 'slowdebug'.
>>>> Folks use an option in a test that requires 
>>>> '-XX:+UnlockDiagnosticVMOptions',
>>>> but forget to include it in the test's run statement and we end up 
>>>> with a test failure in 'release' bits.
>>>>
>>>> I would prefer that 'UnlockExperimentalVMOptions' did not introduce 
>>>> the same path to failing tests.
>>>
>>> I tried to change UnlockDiagnosticVMOptions to be false, and got a 
>>> wall of opposition:
>>>
>>> See: https://bugs.openjdk.java.net/browse/JDK-8153783
>>>
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-January/029882.html 
>>>
>>
>> I would not say "a wall of opposition". You got almost equal amounts
>> of "yea" and "nay". I was a "yea" and I have been continuing to train
>> my fingers (and my scripts) to do the right thing.
> 
> You should have seen my slack channel at that time. :)  Maybe the "wall" 
> was primarily from a couple of people who strongly objected.
>>
>> Interestingly, David H was a "nay" on changing UnlockDiagnosticVMOptions
>> to be 'false', but appears to be leaning toward "nay" on changing
>> UnlockExperimentalVMOptions to 'trueInDebug'...
>>
> 
> I think he's mostly just asking the question.  We'll see what he answers 
> later.

Yes I'm just asking the question. I don't think changing this buys us 
much other than "it's now the same as for diagnostic flags". Testing 
these flags can (and probably should) be handled explicitly.

I looked back at the discussion on JDK-8153783 (sorry can't recall what 
may have been said in slack) and I'm not sure what my specific concern 
was then. From a testing perspective if you use an experimental or 
diagnostic flag then you should remember to explicitly unlock it in the 
test setup. Not having trueInDebug catches when you forget that and only 
test in a debug build.

Cheers,
David
-----

>>
>>> I think the same exact arguments should apply to 
>>> UnlockExperimentalVMOptions.  I'd like to hear from someone that uses 
>>> experimental options on ZGC or shenandoah, since those have the most 
>>> experimental options.
>>
>> I agree that the same arguments apply to UnlockExperimentalVMOptions.
>> For consistency's sake if anything, they should be the same.
>>
>>
>>> The reason that I made it trueInDebug is so that the command line 
>>> option range test would test these options.  Otherwise a more hacky 
>>> solution could be done, including adding the parameter 
>>> -XX:+UnlockExperimentalVMOptions to all the VM option range tests. 
>>> I'd rather not do this.
>>
>> Can explain this a bit more? Why would a default value of 'false' mean 
>> that
>> the command line option range test would not test these options?
> 
> So the command line option tests do - java -XX:+PrintFlagsRanges 
> -version and collect the flags that come out, parse the ranges, and then 
> run java with each of these flags with the limits of the range (unless 
> the limit is INT_MAX).  Some flags are excluded explicitly because they 
> cause problems.
> 
> The reason that SymbolTableSize escaped the testing, is because it 
> wasn't reported with -XX:+PrintFlagsRanges.  You'd need 
> -XX:+UnlockExperimentalVMOptions in the java command to gather the 
> flags, and then pass it to all the java commands to test the ranges. 
> It's not that bad, just a bit gross.
> 
> In any case, I think the experimental flags ranges should be tested. I'm 
> glad/amazed that more didn't fail when I turned it on in my testing.
> 
>>
>> In any case, I'm fine if you want to move forward with changing the
>> default of UnlockExperimentalVMOptions to 'trueInDebug'.
>>
> 
> Okay, we'll wait to see whether I get a wall of opposition or support. I 
> still think it should be by default the same as UnlockDiagnosticVMoptions.
> 
> Thanks!
> Coleen
> 
>> Dan
>>
>>
>>>
>>> Thanks,
>>> Coleen
>>>
>>>>
>>>> Dan
>>>>
>>>>
>>>>>>
>>>>>> Functional change seems fine. Is it worth adding a clarifying 
>>>>>> comment to:
>>>>>>
>>>>>> +          range(minimumSymbolTableSize, 16777216ul)     \
>>>>>>
>>>>>> with:
>>>>>>
>>>>>> +          range(minimumSymbolTableSize, 16777216ul /* 2^24 */) 
>>>>>>                \
>>>>>
>>>>> Let me see if the X macro allows that and I could also add that to 
>>>>> StringTableSize (which is not experimental option).
>>>>> Thanks,
>>>>> Coleen
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>> On 23/07/2019 4:45 am, coleen.phillimore at oracle.com wrote:
>>>>>>> Summary: Increase max size for SymbolTable and fix experimental 
>>>>>>> option range.  Make experimental options trueInDebug so they're 
>>>>>>> tested by the command line option testing
>>>>>>>
>>>>>>> open webrev at 
>>>>>>> http://cr.openjdk.java.net/~coleenp/2019/8227123.01/webrev
>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8227123
>>>>>>>
>>>>>>> Tested locally with default and -XX:+UseZGC since ZGC has a lot 
>>>>>>> of experimental options.  I didn't test with shenanodoah.
>>>>>>>
>>>>>>> I will test with hs-tier1-3 before checking in.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>
>>>>
>>>
>>
>