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

David Holmes david.holmes at oracle.com
Tue Jul 21 02:03:44 UTC 2015

On 21/07/2015 3:02 AM, Derek White wrote:
> 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 :-)

Name aside you didn't comment on my main comment here: an obsolete 
option has already been removed from globals etc and does nothing.

>>  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.

Sounds fine but again we don't normally document test strategies in the 
source code.

>>  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.

So that variable should really be obsoleted_or_deprecated_in ?

>>  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.

But what you are seeing before April is the result of hotspot express 
(at least in a large part). Now that we don't have to support that we 
don't have legacy lists to maintain. The code could even be changed upon 
detecting that current JDK version == "until" version to report "Hey you 
forgot to update the table!" ;-)

My point is that the comments should reflect how we intend to use these, 
not give the impression that keeping eliminated options in the table is 
the expected thing to do.

>> 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.

But again comments should reflect expected usage - if we reach the 
"until" version of a deprecated option that wasn't obsoleted then 
something has probably gone wrong.

>> 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.

Yes I like that too.
>> 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.

I need to think more on that. It is hard to see the overall control flow 
for argument processing.


>> 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

More information about the hotspot-dev mailing list