RFR: 8235966: Process obsolete flags less aggressively
David Holmes
david.holmes at oracle.com
Tue Jan 21 23:10:37 UTC 2020
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