RFR: 8235966: Process obsolete flags less aggressively
David Holmes
david.holmes at oracle.com
Thu Jan 23 22:51:59 UTC 2020
Ping! Anyone?
David
On 22/01/2020 12:06 pm, David Holmes wrote:
> Can I get a second review please?
>
> Thanks,
> David
>
> On 22/01/2020 9:10 am, David Holmes wrote:
>> Thanks Dan.
>>
>> Will fix the "typo" - though not actually a typo as it was a
>> deliberate (mis)use of "monthly". I have always quantified "monthly"
>> this way, but seems it is not correct. :)
>>
>> Cheers,
>> David
>>
>> On 22/01/2020 1:37 am, Daniel D. Daugherty wrote:
>>> On 1/19/20 8:49 PM, David Holmes wrote:
>>>> gtest version:
>>>>
>>>> http://cr.openjdk.java.net/~dholmes/8235966/webrev.v4/
>>>
>>> src/hotspot/share/runtime/arguments.hpp
>>> No comments.
>>>
>>> src/hotspot/share/runtime/arguments.cpp
>>> L715: // check for stale flags when we hit build 20 (which is
>>> far enough into the 6 monthly
>>> typo: s/monthly/month/
>>>
>>> test/hotspot/gtest/runtime/test_special_flags.cpp
>>> Nice!
>>>
>>> Thumbs up. I don't need to see a new webrev to fix the typo.
>>>
>>> Dan
>>>
>>>
>>>>
>>>> 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