[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-dev
Subject: Re: RFR (S) 8227123: Assertion failure when setting SymbolTableSize larger than 2^17 (131, 072)
From: Coleen Phillimore <coleen.phillimore () oracle ! com>
Date: 2019-07-29 23:41:42
Message-ID: 3BC0A9B6-C6E3-4D9E-A607-055693343F0B () oracle ! com
[Download RAW message or body]
Thanks David!
Coleen
> On Jul 29, 2019, at 6:22 PM, David Holmes <david.holmes@oracle.com> wrote:
>
> Hi Coleen,
>
> Updates LGTM too!
>
> Thanks,
> David
>
> > On 26/07/2019 7:53 am, coleen.phillimore@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@oracle.com wrote:
> > >
> > >
> > > > On 7/24/19 9:20 AM, David Holmes wrote:
> > > > > On 24/07/2019 11:04 pm, coleen.phillimore@oracle.com wrote:
> > > > > > On 7/23/19 10:20 PM, David Holmes wrote:
> > > > > > > On 24/07/2019 1:48 am, coleen.phillimore@oracle.com wrote:
> > > > > > > > On 7/23/19 11:30 AM, Daniel D. Daugherty wrote:
> > > > > > > > > On 7/23/19 11:09 AM, coleen.phillimore@oracle.com wrote:
> > > > > > > > > > On 7/23/19 9:45 AM, Daniel D. Daugherty wrote:
> > > > > > > > > > > On 7/23/19 7:03 AM, coleen.phillimore@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@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
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> > >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic