RFR: 8235966: Process obsolete flags less aggressively
Igor Ignatyev
igor.ignatyev at oracle.com
Thu Jan 23 23:00:13 UTC 2020
Hi David,
in test_special_flags.cpp, I'd put the whole TEST_VM(special_flags, verify_special_flags) in #ifdef ASSERT / endif, other than that LGTM.
-- Igor
> On Jan 23, 2020, at 2:51 PM, David Holmes <david.holmes at oracle.com> wrote:
>
> 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