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

Mon Jul 29 23:41:42 UTC 2019

Thanks David!
Coleen

> On Jul 29, 2019, at 6:22 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Coleen,
> 
> Updates LGTM too!
> 
> Thanks,
> David
> 
>> On 26/07/2019 7:53 am, coleen.phillimore at oracle.com wrote:
>> After some offline polling of various people, I'm going to withdraw the UnlockExperimentalOptions change to trueInDebug, and fixed the options test.
>> http://cr.openjdk.java.net/~coleenp/2019/8227123.02/webrev/index.html
>> These test changes would have found the original bug.
>> Thanks,
>> Coleen
>>> On 7/24/19 9:52 AM, coleen.phillimore at oracle.com wrote:
>>> 
>>> 
>>>> 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
>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>> 
>>> 