RFR: 8066821(S) Enhance command line processing to manage deprecating and obsoleting -XX command line arguments

Derek White derek.white at oracle.com
Mon Jul 20 17:02:08 UTC 2015


Hi David,

Thanks for looking this over.

On 7/20/15 5:29 AM, David Holmes wrote:
> Hi Derek,
>
> Sorry but I'm finding this a bit confused and inconsistent. Comments 
> below.
>
> On 18/07/2015 3:30 AM, Derek White wrote:
>> Request for review:
>>
>> [This updated webrev is being sent to wider audience, and has been
>> merged with Gerard's option constraints check-in. Also factored out
>> -XX:+AggressiveHeap processing into it's own chapter. I mean function 
>> :-)]
>>
>> http://cr.openjdk.java.net/~drwhite/8066821/webrev.06/
>> https://bugs.openjdk.java.net/browse/JDK-8066821
>
> argument.cpp:
>
>  258  *               been not scheduled).
>
> -> not been scheduled

OK.
>  260 *     OBSOLETE: An option may be removed (and deleted from 
> globals.hpp), but still be accepted on the command
>  261  *               line. A warning is printed to let the user know 
> that support may be removed in the future.
>
>
> Isn't this the stronger case that support has already been removed 
> (the option does nothing) and it will be removed at the next major 
> release. At the start of a major release we should be able to delete 
> all entries from the obsolete table - else it wasn't obsolete but 
> deprecated.
>
> Not sure "obsolete" is the right term here - obsolete things still 
> function. For us an obsolete option does nothing (it exists only in 
> the obsolete table).
It's not a great name, but that what the previous code called it. It's a 
"I'll pretend you didn't say that" option, like when a teenager hears an 
adult use out-of-date slang ("groovy!"). How about a "condescending" 
option :-)
>
>  276  * Tests:  Aliases are tested in VMAliasOptions.java.
>  277  *         Deprecated options are tested in 
> VMDeprecatedOptions.java.
>  278  *         Obsolete options are tested in various files.
>
> We don't normally document this kind of thing in the source.

I'm trying to head off unnecessary one-off test files for each alias and 
deprecated option. For example TestNoParNew.java and 
TestDefaultMaxRAMFraction.java. And I think that there should be one 
test file for obsolete options also (perhaps expanding 
ObsoleteFlagErrorMessage.java), instead of a bunch of separate test 
files, but that is not in this webrev.
>
>  281 // Obsolete or deprecated -XX flag.
>
> I can tell this is going to get confusing already.
>
>  284   JDK_Version obsoleted_in; // When the warning started (obsolete 
> or deprecated).
>
> But there is a difference: deprecated == warn but still process; 
> obsolete == warn and ignore.
Yes, but the SpecialFlag type is concerned with the common aspect of 
warning. The "ignore" or "process" aspects are handled by the different 
functions that look up the obsolete_jvm_flags and deprecated_jvm_flags 
arrays.
>
>  288 // When a flag is eliminated, it can be added to this list in 
> order to
>  289 // continue accepting this flag on the command-line, while 
> issuing a warning
>  290 // and ignoring the value.  Once the JDK version reaches the 
> 'accept_until'
>  291 // limit, we flatly refuse to admit the existence of the flag.
>  292 static SpecialFlag const obsolete_jvm_flags[] = {
>
> When a flag is eliminated it is gone from this table. As soon as the 
> accept_until version is the current version we wipe the table of all 
> such entries. This should be one of the first things done in a new 
> release.

I completely agree that this is a great plan. But until this April we 
still had obsolete flags listed for JDK 5! The obsolete_jvm_flags table 
did the right thing until the process caught up. In any case, this 
webrev doesn't really change the behavior of the obsolete_jvm_flags table.
>
> 320 // When a flag is deprecated, it can be added to this list in 
> order to issuing a warning when the flag is used.
>  321 // Once the JDK version reaches the 'accept_until' limit, we 
> flatly refuse to admit the existence of the flag.
>  322 static SpecialFlag const deprecated_jvm_flags[] = {
>
> A deprecated flag should be obsoleted before it reaches the 
> accept_until limit.
That's a policy that's under discussion on hotspot-runtime-dev. There 
are certainly option lifecycles that have been approved by our internal 
process that don't follow this proposed policy. The mechanism in this 
webrev was concerned about supporting the plethora of current 
lifecycles, for better or worse.
> Which suggests that when we start a new version we wipe the obsoleted 
> table and move the deprecated options into it.
I can add documentation to describe this case.

If we decide that we'll always treat a deprecated or aliased option as 
obsolete for one extra release, then it might make sense to have a 
combined option lifecycle table. But for now I like the fact that 
options in deprecated_jvm_flags should always have a real variable 
defined in globals.hpp (etc), and options in obsolete_jvm_flags should 
never have a global variable.

> 776 case 1: {
>  777       if (warn) {
>  778         char version[256];
>  779         since.to_string(version, sizeof(version));
>  780         if (real_name != arg) {
>  781           warning("Option %s was deprecated in version %s and 
> will likely be removed in a future release. Use option %s instead.",
>  782                   arg, version, real_name);
>  783         } else {
>  784           warning("Option %s was deprecated in version %s and 
> will likely be removed in a future release.",
>  785                   arg, version);
>  786         }
>
> This isn't distinguishing between deprecated and obsoleted ???

Yes, handle_aliases_and_deprecation() doesn't handle obsolete options 
(or it would have had a longer name :-) Maybe it should handle that 
case, but it would have complicated the control flow in the caller. I 
have another proposed change in the works that simplifies the caller 
quite a bit that would make the refactoring simpler.
>
> 997       warning("Ignoring option %s; support was removed in %s", 
> stripped_argname, version);
>
> You have changed a handful of warnings so they start with a capital 
> letter, and have changed the associated tests. But this seems a 
> pointless "convention" because we have dozens of warnings that don't 
> start with a capital.
The new deprecation and alias warnings replaces these various messages:
     warning("The UseParNewGC flag is deprecated and will likely be 
removed in a future release");
     warning("Using MaxGCMinorPauseMillis as minor pause goal is 
deprecated and will likely be removed in future release");
     warning("DefaultMaxRAMFraction is deprecated and will likely be 
removed in a future release. Use MaxRAMFraction instead.");
     jio_fprintf(defaultStream::error_stream(),
         "Please use -XX:MarkStackSize in place of -XX:CMSMarkStackSize 
or -XX:G1MarkStackSize in the future\n");
      jio_fprintf(defaultStream::error_stream(),
         "Please use -XX:ConcGCThreads in place of 
-XX:ParallelMarkingThreads or -XX:ParallelCMSThreads in the future\n");
      jio_fprintf(defaultStream::error_stream(),
          "Please use -XX:MarkStackSizeMax in place of 
-XX:CMSMarkStackSizeMax in the future\n");
      jio_fprintf(defaultStream::output_stream(),
           "CreateMinidumpOnCrash is replaced by CreateCoredumpOnCrash: 
CreateCoredumpOnCrash is on\n");

It made sense to have the case of the obsolete messages match the 
deprecation and aliases messages. Arguably I got carried away with 
changing the messages for UseAdaptiveSizePolicy, RequireSharedSpaces, 
and ScavengeRootsInCode warnings (but they didn't force changes to tests).

Thanks again for you comments!

  - Derek
>
> Thanks,
> David
> -----
>
>> This webrev adds support for handling "/deprecated/" -XX options
>> (options that still *do* something but are planned for removal) and
>> "/alias/" options (alternate names for other -XX options) by simply
>> adding entries to the deprecated_jvm_flags and/or aliased_jvm_flags
>> tables. This follows the example of the existing obsolete_jvm_flags 
>> table.
>>
>> This replaces a lot of ad-hoc and occasionally wrong code in
>> arguments.cpp as well as supporting automatically disabling options
>> after a certain version.
>>
>> Tests:
>> Deprecated and alias options can be tested by adding entries to tables
>> in new tests:
>>    VMAliasOptions.java
>>    VMDeprecatedOptions.java
>>
>> The new tests subsume these existing tests:
>>    test/gc/startup_warnings/TestDefaultMaxRAMFraction.java
>>    test/gc/startup_warnings/TestNoParNew.java
>>
>> Tests run:
>>      jprt
>>      jtreg (investigating errors in resource mgmt tests running on SE
>> embedded that seems unrelated to these changes.)
>>
>> Thanks,
>>
>>   - Derek
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20150720/8c17e160/attachment-0001.html>


More information about the hotspot-gc-dev mailing list