RFR: 8235966: Process obsolete flags less aggressively
David Holmes
david.holmes at oracle.com
Mon Jan 20 01:49:59 UTC 2020
gtest version:
http://cr.openjdk.java.net/~dholmes/8235966/webrev.v4/
Bug report updated to show gtest output.
Thanks,
David
On 20/01/2020 7:46 am, David Holmes wrote:
> Hi Dan,
>
> On 18/01/2020 1:37 am, Daniel D. Daugherty wrote:
>> On 1/16/20 12:57 AM, David Holmes wrote:
>>> Getting back to this ...
>>
>> You added this update to the bug report:
>>
>>> Update: after further discussion it has been proposed that we use the
>>> build number as the trigger for a whitebox or gtest that performs the
>>> currently disabled full verification of the flag table. So if a flag
>>> has not be obsoleted or expired as it should by build N** then we
>>> fail the gtest. This will complement the relaxing of the obsoletion
>>> check at the start of the release cycle.
>>
>> I was expecting a new test in this latest webrev that would start failing
>> at Build 20... let me see what the latest webrev says...
>
> Mea culpa. When I started thinking about the test it was evident the
> test logic would just involve the existing commented out warning logic.
> So I thought lets just turn those warnings on at build 20 but ...
>
>>>
>>> Please see updated webrev at:
>>>
>>> http://cr.openjdk.java.net/~dholmes/8235966/webrev/
>>
>> src/hotspot/share/runtime/arguments.cpp
>> No comments.
>>
>> So the changes are the same as the last round with the addition of
>> enabling the following at B20:
>>
>> L755: warning("Global variable for obsolete special flag
>> entry \"%s\" should be removed", flag.name);
>> L769: warning("Global variable for expired flag entry \"%s\"
>> should be removed", flag.name);
>>
>>
>>> Apologies as I mistakenly overwrote the original instead of creating v3.
>>>
>>> This version expands on the original proposal by uncommenting the
>>> warnings about obsolete/expired flags that have not been removed from
>>> globals*.hpp, so that we don't forget to do this work. However these
>>> warnings are only enabled from build 20. I used 20 as being approx
>>> 2/3 through the release cycle - long enough that the work should have
>>> been performed by then, whilst still leaving time to perform the work
>>> before RDP2. Of course we can tweak this number if there are issues
>>> with that choice.
>>
>> Okay... but doesn't this mean that every test would issue these warnings
>> as of B20 if we have not completed the work? So we have the potential of
>> a raft (some unknown number) of test failures due to unexpected output in
>> the form of these warning messages. And worse, these failures would be
>> unrelated to actual issue... :-(
>
> ... as you note the end result is not really what we want - a single
> clearly failing test.
>
>> How about adding a diagnostic flag that enables these two warning
>> messages (in addition to the B20 check). Add a single test that runs:
>>
>> java -XX:+UnlockDiagnosticVMOptions
>> -XX:+FlagShouldNotBeDefinedCheck -version
>>
>> and when we hit B20, if there are still flags that haven't been removed,
>> then the test will fail and we'll have one test that fails (X the number
>> of configs that run the test).
>
> I definitely do not want to add another VM flag :) but will look at how
> to (re)run that logic as part of a gtest.
>
> Thanks,
> David
>
>> Dan
>>
>>
>>>
>>> Thanks,
>>> David
>>>
>>> On 20/12/2019 8:20 am, David Holmes wrote:
>>>> Thanks Dan.
>>>>
>>>> FTR I've updated the bug report with an extension to this proposal,
>>>> which is to add back the flag table validation checks to use via a
>>>> gtest that we only enable after a certain build in the release cycle
>>>> (it always passes before then). That way we avoid the problems I've
>>>> outlined with the initial version bump but also have a safety net in
>>>> place to ensure we don't forget to actually obsolete/expire flags.
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>> On 19/12/2019 3:37 am, Daniel D. Daugherty wrote:
>>>>> Hi David,
>>>>>
>>>>> On 12/17/19 5:03 PM, David Holmes wrote:
>>>>>> Hi Dan,
>>>>>>
>>>>>> Thanks for taking a look. Updated webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dholmes/8235966/webrev.v2/
>>>>>
>>>>> src/hotspot/share/runtime/arguments.cpp
>>>>> I like the updates to header comment for
>>>>> verify_special_jvm_flags().
>>>>>
>>>>> Thumbs up.
>>>>>
>>>>>
>>>>>>
>>>>>> Discussion below.
>>>>>
>>>>> Replies below.
>>>>>
>>>>>
>>>>>>
>>>>>> On 18/12/2019 1:47 am, Daniel D. Daugherty wrote:
>>>>>>> On 12/16/19 12:16 AM, David Holmes wrote:
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8235966
>>>>>>>> webrev: http://cr.openjdk.java.net/~dholmes/8235966/webrev/
>>>>>>>
>>>>>>> src/hotspot/share/runtime/arguments.cpp
>>>>>>> L745: // if flag has become obsolete it should not
>>>>>>> have a "globals" flag defined anymore.
>>>>>>> L746: if (!version_less_than(JDK_Version::current(),
>>>>>>> flag.obsolete_in)) {
>>>>>>> L747: if (JVMFlag::find_declared_flag(flag.name) !=
>>>>>>> NULL) {
>>>>>>> L748: // Temporarily disable the warning: 8196739
>>>>>>> L749: // warning("Global variable for obsolete
>>>>>>> special flag entry \"%s\" should be removed", flag.name);
>>>>>>> L750: }
>>>>>>> L751: }
>>>>>>> It seems like we've been down a similar road before:
>>>>>>>
>>>>>>> JDK-8196739 Disable obsolete/expired VM flag
>>>>>>> transitional warnings
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8196739
>>>>>>>
>>>>>>> This one may ring a bell... Fixed by dholmes in
>>>>>>> jdk11-b01... :-)
>>>>>>>
>>>>>>> And this followup sub-task to re-enable that warning:
>>>>>>>
>>>>>>> JDK-8196741 Re-enable obsolete/expired VM flag
>>>>>>> transitional warnings
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8196741
>>>>>>>
>>>>>>> was closed as "Won't fix" on 2019.08.02.
>>>>>>>
>>>>>>> So the obvious questions:
>>>>>>>
>>>>>>> - Why is the new warning less problematic to tests that
>>>>>>> don't
>>>>>>> tolerate unexpected output?
>>>>>>
>>>>>> Two different situations. The commented out warning happens
>>>>>> unconditionally when you run the VM and it finds any flag marked
>>>>>> obsolete that hasn't been removed. Hence every single test will
>>>>>> encounter this warning.
>>>>>
>>>>> Ouch on such verbosity.
>>>>>
>>>>>
>>>>>> The situation I am modifying is when a test uses a flag that is
>>>>>> marked for obsoletion. In the majority of cases the flag is
>>>>>> already deprecated and so already issuing a deprecation warning
>>>>>> that the test has to handle. Without my change there would still
>>>>>> be an obsoletion warning, so this test is in for a warning no
>>>>>> matter what.
>>>>>
>>>>> Good that your change only comes into play when the flag is used.
>>>>>
>>>>>
>>>>>> Also note that for hotspot at least we have strived to make tests
>>>>>> tolerate unexpected output. The reason JDK-8196741 was closed as
>>>>>> "won't fix" was because other areas wouldn't commit to doing that.
>>>>>
>>>>> Yup. Got it.
>>>>>
>>>>>
>>>>>>
>>>>>>> - If you move forward with this fix, then I think think
>>>>>>> code
>>>>>>> block needs to be removed or modified or am I missing
>>>>>>> something?
>>>>>>
>>>>>> I've rewritten the comment at the head of verify_special_jvm_flags
>>>>>> to explain why we can't issue a warning, and have deleted the block.
>>>>>
>>>>> Thanks for deleting the stale code.
>>>>>
>>>>>
>>>>>>
>>>>>>> There's a similar commented out check on L757-L765, but
>>>>>>> that one
>>>>>>> is for an expired flag... You might want to
>>>>>>> adjust/delete it also?
>>>>>>
>>>>>> Deleted.
>>>>>
>>>>> Thanks.
>>>>>
>>>>>
>>>>>>
>>>>>>> L753: warning("Special flag entry \"%s\" must be
>>>>>>> explicitly obsoleted before expired.", flag.name);
>>>>>>> L754: success = false;
>>>>>>> nit - s/before expired/before being expired/
>>>>>>> Update: I now see that "style" is in several places in this
>>>>>>> function. I'm not sure what to think here... it grates,
>>>>>>> but I can live with it.
>>>>>>>
>>>>>>> nit - L75[34] indented too much by two spaces.
>>>>>>
>>>>>> Fixed.
>>>>>>
>>>>>>> L962: return real_name;
>>>>>>> nit - indented too much by two spaces.
>>>>>>
>>>>>> Fixed.
>>>>>>
>>>>>>>
>>>>>>> Trying to understand the modified logic in argument processing is
>>>>>>> making my head spin...
>>>>>>
>>>>>> Mine too. It took a few attempts to put the logic in the right
>>>>>> place and make adjustments so that it all works as expected for a
>>>>>> correctly specified flag and an erroneous one.
>>>>>
>>>>> I keep trying to convince myself that we're improving this flag and
>>>>> options code with each release... :-)
>>>>>
>>>>>
>>>>>>
>>>>>>> - You've added a call to is_obsolete_flag() in
>>>>>>> handle_aliases_and_deprecation() and is_obsolete_flag()
>>>>>>> is where the new warning is output:
>>>>>>>
>>>>>>> warning("Temporarily processing option %s; support is
>>>>>>> scheduled for removal in %s"
>>>>>>>
>>>>>>> handle_aliases_and_deprecation() is called from six different
>>>>>>> places,
>>>>>>> but the call sites are different based on the argument pattern
>>>>>>> so I
>>>>>>> have (mostly) convinced myself that there should not be any
>>>>>>> duplicate
>>>>>>> warning lines.
>>>>>>
>>>>>> Right - handle_aliases_and_deprecation is only called for a
>>>>>> syntactically correct flag based on those patterns. It normally
>>>>>> filters out obsoleted/expired flags and lets them fall through to
>>>>>> later error processing (in process_argument after parse_arg
>>>>>> returns false). That error processing is where the normal
>>>>>> obsoletion check is performed. So I had to not filter the flag in
>>>>>> handle_aliases_and_deprecation in this case, but still produce the
>>>>>> warning for a malformed flag. E.g.
>>>>>>
>>>>>> java -XX:+UseParallelOldGC -version
>>>>>> Java HotSpot(TM) 64-Bit Server VM warning: Temporarily processing
>>>>>> option UseParallelOldGC; support is scheduled for removal in 15.0
>>>>>> java version "15-internal" 2020-09-15
>>>>>>
>>>>>> java -XX:UseParallelOldGC -version
>>>>>> Java HotSpot(TM) 64-Bit Server VM warning: Temporarily processing
>>>>>> option UseParallelOldGC; support is scheduled for removal in 15.0
>>>>>> Missing +/- setting for VM option 'UseParallelOldGC'
>>>>>> Error: Could not create the Java Virtual Machine.
>>>>>
>>>>> Thanks for the example. That helps a lot.
>>>>>
>>>>>
>>>>>>
>>>>>>> So I now understand the new logic that allows an obsoleted option
>>>>>>> to be specified with a warning as long as the option still exists.
>>>>>>> I'm good with the technical change, but...
>>>>>>>
>>>>>>> I'm worried about tests that don't tolerate the new warning mesg,
>>>>>>> i.e., why wouldn't this become an issue again:
>>>>>>>
>>>>>>> JDK-8196739 Disable obsolete/expired VM flag transitional warnings
>>>>>>
>>>>>> Explained above.
>>>>>
>>>>> Yup and thanks.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> When a flag is marked as obsolete in the special-flags table we
>>>>>>>> will ignore it and issue a warning that it is being ignored, as
>>>>>>>> soon as we bump the version of the JDK. That means that any
>>>>>>>> tests still using the obsolete flag may start to fail, leading
>>>>>>>> to a surge of test failures at the start of a release cycle. For
>>>>>>>> example for JDK 15 we have a whole bunch of JFR tests that fail
>>>>>>>> because they still try to work with UseParallelOldGC. In another
>>>>>>>> case runtime/cds/appcds/FieldLayoutFlags.java passes only be
>>>>>>>> accident.
>>>>>>>>
>>>>>>>> When a flag is marked as obsolete for a release, all code
>>>>>>>> involving that flag (including tests that use it) must be
>>>>>>>> updated within that release and the flag itself removed. Whilst
>>>>>>>> this is typically scheduled early in a release cycle it isn't
>>>>>>>> reasonable to expect it to all occur within the first couple of
>>>>>>>> days of the release cycle, nor do we want to have to ProblemList
>>>>>>>> a bunch of tests when they start failing.
>>>>>>>>
>>>>>>>> What I propose is to instead allow an obsolete flag to continue
>>>>>>>> to be processed as long as that code removal has not actually
>>>>>>>> occurred - with an adjusted warning. The change I propose:
>>>>>>>>
>>>>>>>> - only treats an obsolete flag as obsolete if the flag cannot be
>>>>>>>> found
>>>>>>>> - added a new flag verification rule that disallows obsoletion
>>>>>>>> in an undefined version, but expiration in a specific version
>>>>>>>> i.e. we must always explicitly obsolete a flag before we expire it.
>>>>>>>>
>>>>>>>> The only downside here is that if we actually forget to file an
>>>>>>>> issue for the actual obsoletion work we may not notice via
>>>>>>>> testing. Of course whenever a change is made to the flags table
>>>>>>>> to add an entry then the issue to do the obsoletion should be
>>>>>>>> filed at the same time.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
>>>>>>>
>>>>>
>>
More information about the hotspot-dev
mailing list