RFR: 8235966: Process obsolete flags less aggressively
David Holmes
david.holmes at oracle.com
Wed Jan 22 02:06:23 UTC 2020
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