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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Wed Jul 24 13:52:45 UTC 2019 


On 7/24/19 9:20 AM, David Holmes wrote:
> 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.

The same is true for diagnostic options.  I'd be surprised if testing in 
release made a difference though, except taking more time.

Coleen

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