<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi David,<br>
<br>
Thanks for looking this over.<br>
<br>
On 7/20/15 5:29 AM, David Holmes wrote:<br>
</div>
<blockquote cite="mid:55ACBF85.9080404@oracle.com" type="cite">Hi
Derek,
<br>
<br>
Sorry but I'm finding this a bit confused and inconsistent.
Comments below.
<br>
<br>
On 18/07/2015 3:30 AM, Derek White wrote:
<br>
<blockquote type="cite">Request for review:
<br>
<br>
[This updated webrev is being sent to wider audience, and has
been
<br>
merged with Gerard's option constraints check-in. Also factored
out
<br>
-XX:+AggressiveHeap processing into it's own chapter. I mean
function :-)]
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8066821/webrev.06/">http://cr.openjdk.java.net/~drwhite/8066821/webrev.06/</a>
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8066821">https://bugs.openjdk.java.net/browse/JDK-8066821</a>
<br>
</blockquote>
<br>
argument.cpp:
<br>
<br>
258 * been not scheduled).
<br>
<br>
-> not been scheduled
<br>
</blockquote>
<br>
OK.<br>
<blockquote cite="mid:55ACBF85.9080404@oracle.com" type="cite"> 260
* OBSOLETE: An option may be removed (and deleted from
globals.hpp), but still be accepted on the command
<br>
261 * line. A warning is printed to let the user
know that support may be removed in the future.
<br>
<br>
<br>
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.
<br>
<br>
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).<br>
</blockquote>
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 :-)<br>
<blockquote cite="mid:55ACBF85.9080404@oracle.com" type="cite">
<br>
276 * Tests: Aliases are tested in VMAliasOptions.java.
<br>
277 * Deprecated options are tested in
VMDeprecatedOptions.java.
<br>
278 * Obsolete options are tested in various files.
<br>
<br>
We don't normally document this kind of thing in the source.
<br>
</blockquote>
<br>
I'm trying to head off
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
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.
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<blockquote cite="mid:55ACBF85.9080404@oracle.com" type="cite">
<br>
281 // Obsolete or deprecated -XX flag.
<br>
<br>
I can tell this is going to get confusing already.
<br>
<br>
284 JDK_Version obsoleted_in; // When the warning started
(obsolete or deprecated).
<br>
<br>
But there is a difference: deprecated == warn but still process;
obsolete == warn and ignore.
<br>
</blockquote>
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.<br>
<blockquote cite="mid:55ACBF85.9080404@oracle.com" type="cite">
<br>
288 // When a flag is eliminated, it can be added to this list in
order to
<br>
289 // continue accepting this flag on the command-line, while
issuing a warning
<br>
290 // and ignoring the value. Once the JDK version reaches the
'accept_until'
<br>
291 // limit, we flatly refuse to admit the existence of the
flag.
<br>
292 static SpecialFlag const obsolete_jvm_flags[] = {
<br>
<br>
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.</blockquote>
<br>
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.
<blockquote cite="mid:55ACBF85.9080404@oracle.com" type="cite">
<br>
320 // When a flag is deprecated, it can be added to this list in
order to issuing a warning when the flag is used.
<br>
321 // Once the JDK version reaches the 'accept_until' limit, we
flatly refuse to admit the existence of the flag.
<br>
322 static SpecialFlag const deprecated_jvm_flags[] = {
<br>
<br>
A deprecated flag should be obsoleted before it reaches the
accept_until limit.</blockquote>
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.<br>
<blockquote cite="mid:55ACBF85.9080404@oracle.com" type="cite">
Which suggests that when we start a new version we wipe the
obsoleted table and move the deprecated options into it.
<br>
</blockquote>
I can add documentation to describe this case.<br>
<br>
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.<br>
<br>
<blockquote cite="mid:55ACBF85.9080404@oracle.com" type="cite">776
case 1: {
<br>
777 if (warn) {
<br>
778 char version[256];
<br>
779 since.to_string(version, sizeof(version));
<br>
780 if (real_name != arg) {
<br>
781 warning("Option %s was deprecated in version %s and
will likely be removed in a future release. Use option %s
instead.",
<br>
782 arg, version, real_name);
<br>
783 } else {
<br>
784 warning("Option %s was deprecated in version %s and
will likely be removed in a future release.",
<br>
785 arg, version);
<br>
786 }
<br>
<br>
This isn't distinguishing between deprecated and obsoleted ???
<br>
</blockquote>
<br>
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.<br>
<blockquote cite="mid:55ACBF85.9080404@oracle.com" type="cite">
<br>
997 warning("Ignoring option %s; support was removed in %s",
stripped_argname, version);
<br>
<br>
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.
<br>
</blockquote>
The new deprecation and alias warnings replaces these various
messages:<br>
warning("The UseParNewGC flag is deprecated and will likely be
removed in a future release");<br>
warning("Using MaxGCMinorPauseMillis as minor pause goal is
deprecated and will likely be removed in future release");<br>
warning("DefaultMaxRAMFraction is deprecated and will likely be
removed in a future release. Use MaxRAMFraction instead.");<br>
jio_fprintf(defaultStream::error_stream(),<br>
"Please use -XX:MarkStackSize in place of
-XX:CMSMarkStackSize or -XX:G1MarkStackSize in the future\n");<br>
jio_fprintf(defaultStream::error_stream(),<br>
"Please use -XX:ConcGCThreads in place of
-XX:ParallelMarkingThreads or -XX:ParallelCMSThreads in the
future\n");<br>
jio_fprintf(defaultStream::error_stream(),<br>
"Please use -XX:MarkStackSizeMax in place of
-XX:CMSMarkStackSizeMax in the future\n");<br>
jio_fprintf(defaultStream::output_stream(),<br>
"CreateMinidumpOnCrash is replaced by
CreateCoredumpOnCrash: CreateCoredumpOnCrash is on\n");<br>
<br>
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).<br>
<br>
Thanks again for you comments!<br>
<br>
- Derek<br>
<blockquote cite="mid:55ACBF85.9080404@oracle.com" type="cite">
<br>
Thanks,
<br>
David
<br>
-----
<br>
<br>
<blockquote type="cite">This webrev adds support for handling
"/deprecated/" -XX options
<br>
(options that still *do* something but are planned for removal)
and
<br>
"/alias/" options (alternate names for other -XX options) by
simply
<br>
adding entries to the deprecated_jvm_flags and/or
aliased_jvm_flags
<br>
tables. This follows the example of the existing
obsolete_jvm_flags table.
<br>
<br>
This replaces a lot of ad-hoc and occasionally wrong code in
<br>
arguments.cpp as well as supporting automatically disabling
options
<br>
after a certain version.
<br>
<br>
Tests:
<br>
Deprecated and alias options can be tested by adding entries to
tables
<br>
in new tests:
<br>
VMAliasOptions.java
<br>
VMDeprecatedOptions.java
<br>
<br>
The new tests subsume these existing tests:
<br>
test/gc/startup_warnings/TestDefaultMaxRAMFraction.java
<br>
test/gc/startup_warnings/TestNoParNew.java
<br>
<br>
Tests run:
<br>
jprt
<br>
jtreg (investigating errors in resource mgmt tests running
on SE
<br>
embedded that seems unrelated to these changes.)
<br>
<br>
Thanks,
<br>
<br>
- Derek
<br>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>