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.
Thanks,
David
>>
>> 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