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:07:33 UTC 2019 

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

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