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

David Holmes david.holmes at oracle.com
Wed Jul 24 13:20:00 UTC 2019 

On 24/07/2019 11:04 pm, coleen.phillimore at oracle.com wrote:
> On 7/23/19 10:20 PM, David Holmes wrote:
>> 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 disagree.  I don't think we should test these flags explicitly when we 
> have a perfectly good test for all the flags, that should be enabled. 
> Which is what my change does.

Your change only causes the experimental flags to be tested in debug 
builds. I would argue they should also be tested in product builds, 
hence the need to be explicit about it.

David
-----

>>
>> 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.
> 
> Yes, that was the rationale for making it 'false' rather than 
> 'trueInDebug'.  People were adding tests with a diagnostic option and it 
> was failing in product mode because the Unlock flag wasn't present.  The 
> more vocal side of the question didn't want to have to add the Unlock 
> flag for all their day to day local testing.   I assume the same 
> argument can be made for the experimental options.
> 
> It would be good to hear the opinion from someone who uses these 
> options.   This is degenerated into an opinion question, and besides 
> being able to cleanly test these options, neither one of us uses or 
> tests experimental options as far as I can tell.  I see tests from the 
> Compiler and GC components.  What do other people think?
> 
> Thanks,
> Coleen
> 
>>
>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>

More information about the hotspot-dev mailing list