<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
Hi,<br>
<br>
I'm happy for more clarity in the process of removing command line
arguments. I have some questions and comments below.<br>
<br>
<div class="moz-cite-prefix">On 6/26/15 4:09 PM, Karen Kinnear
wrote:<br>
</div>
<blockquote
cite="mid:87B07B03-3423-4738-9617-0C7B72A74A6E@oracle.com"
type="cite">
<pre wrap="">Derek,
I am really glad you are looking into this. I expanded this to the runtime folks in case many of them have not yet seen this.
Mary just forwarded this and I believe you haven't checked it in yet, so perhaps still time to discuss and make sure
we all are together on the deprecation policy.
The runtime team was discussing at staff this week how we handle deprecating jvm command-line options as
we are looking to do more clean up in the future.
So our internal change control process classifies our exported interfaces into three categories:
External: command-line flags e.g. -verbose:gc, -Xmx, ...
This includes any commercial flags
Private: -XX flags documented (e.g. performance tuning or troubleshooting)
I would assume this would include -XX product, experimental flags and manageable flags
Internal: undocumented -XX flags
I would assume this would include -XX develop and diagnostic flags
(please correct me if my assumptions are wrong folks)</pre>
</blockquote>
<br>
This is a good categorization. Although I think there's some grey
area between External and Private, where some XX options are so
commonly used they should be considered External. Some of the GC
options may fall into this category like UseParNewGC.<br>
<br>
<blockquote
cite="mid:87B07B03-3423-4738-9617-0C7B72A74A6E@oracle.com"
type="cite">
<pre wrap="">
The way I understand that we handle private -XX options today is a 2-step removal: (obsolete_jvm_flags - where the
release number is in a table and could be undefined)
Using your helpful taxonomy from <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-806682">https://bugs.openjdk.java.net/browse/JDK-806682</a>:
Today: private -XX options use 2-step removal (obsolete_jvm_flags)
release 1: Deprecate & Obsolete - warn about the option but do nothing with it (we can remove all code that supports it)
release 2: Dead - unrecognized
- the point of the 2-step is to give customers time to modify any scripts they use
I believe we have very rarely removed External flags - since customers, licensees, etc. may expect them.
Mini-proposal:
1) I would recommend that we add a future set of changes to add consistent handling for the External flags -
so that they would follow a three-step removal:
release 1: Deprecate & Handle - warn and keep supporting
release 2: Deprecate & Obsolete - warn and do nothing
release 3: Dead - unrecognized
2) For the Internal flags - I think it would be kindest to customers and not a huge amount of additional work if
we were to follow the Private model of using a 2 step.
3) leave the Private flags with the current 2-step removal</pre>
</blockquote>
<br>
Yes, this reflects our current model.<br>
<blockquote
cite="mid:87B07B03-3423-4738-9617-0C7B72A74A6E@oracle.com"
type="cite">
<pre wrap="">
4) add support for aliasing - for any of them
So that if you are doing aliasing, you would follow the model you are adding
release 1: Deprecated & Handled - i.e. warn and still support (and set the information for the new alias)
release 2: Dead - unrecognized
5) Could we possibly take the two flags that followed a different model already, i.e. moving to
Deprecated & Handled and handle those as mistakes rather than part of a new general model?</pre>
</blockquote>
<br>
So this is my question which is around the code review in question.
Note, Derek, can you resend the RFR to
<a class="moz-txt-link-abbreviated" href="mailto:hotspot-dev@openjdk.java.net">hotspot-dev@openjdk.java.net</a> so it gets to all of us (even the
compiler team may want to share in the mechanism).<br>
<br>
Why are UseParNewGC and MaxGCMinorPauseMillis deprecated and handled
and not deprecated and obsolete? I don't actually see why have the
distinction here?<br>
<br>
Did these flags follow the deprecation procedure below? Why not
just make them obsolete, why have this other mechanism? I may be
asking the same question as Karen.<br>
<br>
I think the aliased flags could have a deprecate and handle model
(where handle == alias) in which case you only need one table for
the aliased flags, and you need to keep them in globals.hpp so
they're parsed properly.<br>
<meta charset="utf-8">
<blockquote
cite="mid:87B07B03-3423-4738-9617-0C7B72A74A6E@oracle.com"
type="cite">
<pre wrap="">
Or do you think we will see more cases other than aliasing in which we would want to
release 1: Deprecate & Handle and then release 2: Dead
rather than release 1: Deprecate & Obsolete and then 2: Dead
or rather than a 3 step like the External option proposal above?</pre>
</blockquote>
<br>
I think for the aliased flags you want the first option here
(deprecate&handle), right? But that's only because you need to
keep the option in globals.hpp so it parses correctly, otherwise the
second option (deprecate&obsolete) would be the preference?<br>
<br>
The options in the GC that are being deprecated and handled have had
warnings about them for a while, so making them obsolete doesn't
feel too soon.<br>
<br>
Also, I agree with Kim's comment below that your comment lines are
too long. My fonts are too big to have windows this wide.<br>
<br>
thanks,<br>
Coleen<br>
<blockquote
cite="mid:87B07B03-3423-4738-9617-0C7B72A74A6E@oracle.com"
type="cite">
<pre wrap="">
thanks,
Karen
p.s. Note that all of the deprecation needs to
1) work with licensee engineering to ensure we give licensee's a head's up and get feedback
2) file a change control request
- we try to do these both as bulk requests to reduce the processing overhead.
p.p.s. Details
1. Do the warnings print today or are they silent? Just want to make sure we are conscious of how
those are handled if any of this moves to the new unified logging mechanism for which the new default
for "warning" level is to print.
2. "will likely be removed in a future release"? If we have already set the release it will be removed - is this a bit
vague or did I not read closely enough?
On Feb 3, 2015, at 6:30 PM, Derek White wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Request for review (again):
- Updated with Kim's suggestion.
- Stopped double-printing warnings in some cases.
- Initial caps on warning() messages.
Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8066821/webrev.04/">http://cr.openjdk.java.net/~drwhite/8066821/webrev.04/</a>
CR: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8066821">https://bugs.openjdk.java.net/browse/JDK-8066821</a>
Tests: jtreg, jprt
Thanks for looking!
- Derek
On 1/28/15 3:47 PM, Kim Barrett wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On Jan 15, 2015, at 6:34 PM, Derek White <a class="moz-txt-link-rfc2396E" href="mailto:derek.white@oracle.com"><derek.white@oracle.com></a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi All,
I need another review, especially someone from runtime. Coleen, do you want crack at it or can you forward as needed?
Another version for review:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8066821/webrev.01">http://cr.openjdk.java.net/~drwhite/8066821/webrev.01</a>
</pre>
</blockquote>
<pre wrap="">I noticed a bunch of formatting changes that were in the previous
webrev have been eliminated in the .01 webrev. Since I was going to
comment or complain about at least some of them, I have no objection
to having them backed out.
This is not a complete review; I've only gotten part way through the
first file! Unfortunately, I'm swamped with other things right now
and don't seem to be making progress toward finishing. So here's what
I've got so far.
------------------------------------------------------------------------------
src/share/vm/runtime/arguments.cpp
132 int len = (int)strlen(name);
Pre-existing issue.
Why is this using int and casting to int, rather than just using
size_t? The return type for strlen is size_t, as is the argument for
strncmp where len is used, and the other use of len also can/should be
size_t.
[I only noticed this because an earlier webrev tweaked the formatting
of this line.]
------------------------------------------------------------------------------
src/share/vm/runtime/arguments.cpp
235 const char* spec_vendor = "Sun Microsystems Inc.";
236 uint32_t spec_version = 0;
Dead initializers; the variables are immediately reassigned new
values.
This is a pre-existing defect that I wouldn't have even noticed had
the enum for bufsz not been reformatted in a previous webrev. (How's
that for a lesson in being lazy. :-)
------------------------------------------------------------------------------
src/share/vm/runtime/arguments.cpp
253 * -XX arguments are usually defined in globals.hpp, globals_<cpu>.hpp, globals_<os>.hpp, <compiler>_globals.hpp, or <gc>_globals.hpp.
Rather than "are usually defined in" I think better would be something
like "are defined several places, such as".
------------------------------------------------------------------------------
src/share/vm/runtime/arguments.cpp
251 * -XX argument processing:
While the Hotspot style guidelines explicitly don't specify a line
length limit, I find lines that are long enough that they don't fit in
one side of a side-by-side webrev on a reasonable-sized landscape
monitor with a font size I can read without having to scroll the frame
back and forth to be pretty annoying.
------------------------------------------------------------------------------
src/share/vm/runtime/arguments.cpp
419 if (((strncmp(flag_status.name, s, len) == 0) &&
420 (strlen(s) == len)) ||
421 ((s[0] == '+' || s[0] == '-') &&
422 (strncmp(flag_status.name, &s[1], len) == 0) &&
423 (strlen(&s[1]) == len))) {
Pre-existing issue.
The calls to strlen and comparisons to len on lines 420 and 423 could
be replaced with
420: s[len] == '\0'
423: s[len+1] == '\0'
------------------------------------------------------------------------------
src/share/vm/runtime/arguments.cpp
449 size_t len = strlen(flag_status.alias_name);
450 if (((strncmp(flag_status.alias_name, flag_name, len) == 0) &&
451 (strlen(flag_name) == len))) {
There is no point to using strlen() and strncmp() here. Just use
strcmp(), e.g.
if (strcmp(flag_status.alias_name, flag_name) == 0) {
<a class="moz-txt-link-freetext" href="http://stackoverflow.com/questions/14885000/does-strlen-in-a-strncmp-expression-defeat-the-purpose-of-using-strncmp-ov">http://stackoverflow.com/questions/14885000/does-strlen-in-a-strncmp-expression-defeat-the-purpose-of-using-strncmp-ov</a>
------------------------------------------------------------------------------
src/share/vm/runtime/arguments.cpp
412 int i = 0;
413 assert(version != NULL, "Must provide a version buffer");
414 while (special_table[i].name != NULL) {
...
432 i++;
Pre-existing issue.
More readable would be to use a for-loop, e.g.
for (size_t i = 0; special_table[i].name != NULL; ++i) {
...
}
size_t is a better type for i too.
------------------------------------------------------------------------------
src/share/vm/runtime/arguments.cpp
446 int i = 0;
447 while (aliased_jvm_flags[i].alias_name != NULL) {
...
454 i++;
As above, a for-loop would be more readable.
------------------------------------------------------------------------------
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<br>
</body>
</html>