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
>>
More information about the hotspot-dev
mailing list