RFR (S) 8227123: Assertion failure when setting SymbolTableSize larger than 2^17 (131,072)
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Fri Jul 26 13:57:54 UTC 2019
Thanks, Harold!
Coleen
On 7/26/19 8:56 AM, Harold Seigel wrote:
> 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