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

Harold Seigel harold.seigel at oracle.com
Fri Jul 26 12:56:35 UTC 2019 

Looks good!

Thanks, Harold

On 7/25/2019 5:53 PM, 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
>

More information about the hotspot-dev mailing list