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

Daniel D. Daugherty daniel.daugherty at oracle.com
Wed Jul 24 13:52:11 UTC 2019 

On 7/24/19 9:47 AM, coleen.phillimore at oracle.com wrote:
>
>
> On 7/24/19 9:07 AM, Daniel D. Daugherty wrote:
>> On 7/24/19 9:04 AM, 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.
>>>>
>>>> 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?
>>
>> I use experimental options in the various subsystems that I maintain, 
>> but
>> as I said, I'm training my fingers and my scripts to include the various
>> Unlock options...
>>
>> I think the consistency argument is a winner as is the argument that 
>> folks
>> need to test 'release' bits in addition to 'fastdebug' bits. It's 
>> interesting
>> that non-HotSpot folks typically test with 'release' bits and have a 
>> hard
>> time seeing why HotSpot folks always test with 'fastdebug'. It seems 
>> like at
>> least some HotSpot folks only test with 'fastdebug' and not with 
>> 'release'...
>
> fastdebug has the assertions so seems most profitable to test with. 
> For most changes, testing with product/release yields no more 
> information, unless you get lucky with a race.  It's low percentage.   
> Don't misunderstand me though, I think product testing needs to be 
> done but not for individual changes unless you are testing racy code.
>
> Which consistency argument?  That it should be 'false' rather than 
> 'trueInDebug'.

UnlockDiagnosticVMOptions is 'trueInDebug' and that's been settled before
so UnlockExperimentalVMOptions should also be 'trueInDebug'.

Dan


>
> Ok, so 2 to 1 in votes.
>
> Coleen
>>
>> Dan
>>
>> P.S.
>> I suspect that I'm one of the few anal retentive folks that tests all 
>> three
>> build configs: 'release', 'fastdebug' and 'slowdebug', but I have the 
>> advantage
>> of having my own lab with various (somewhat fast) machines... Of 
>> course that
>> monthly power bill is a pain since it seems that I need A/C here in 
>> Orlando...
>> Can't get away with no A/C like in Colorado...
>>
>>
>>>
>>> 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