RFR: 8235966: Process obsolete flags less aggressively
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Jan 21 15:37:03 UTC 2020
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