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:47:03 UTC 2019 


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

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