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

[Daniel D. Daugherty]

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.

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

In any case, I'm fine if you want to move forward with changing the
default of UnlockExperimentalVMOptions to 'trueInDebug'.

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