Runtime command-line deprecation (Was Re: RFR: 8066821(S) Enhance command line processing to manage deprecating and obsoleting -XX command line arguments
Karen Kinnear
karen.kinnear at oracle.com
Fri Jun 26 20:09:08 UTC 2015
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)
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 https://bugs.openjdk.java.net/browse/JDK-806682:
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
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?
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?
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:
> Request for review (again):
>
> - Updated with Kim's suggestion.
> - Stopped double-printing warnings in some cases.
> - Initial caps on warning() messages.
>
> Webrev: http://cr.openjdk.java.net/~drwhite/8066821/webrev.04/
> CR: https://bugs.openjdk.java.net/browse/JDK-8066821
> Tests: jtreg, jprt
>
> Thanks for looking!
>
> - Derek
>
> On 1/28/15 3:47 PM, Kim Barrett wrote:
>> On Jan 15, 2015, at 6:34 PM, Derek White <derek.white at oracle.com> wrote:
>>> 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:
>>> http://cr.openjdk.java.net/~drwhite/8066821/webrev.01
>> 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) {
>>
>> http://stackoverflow.com/questions/14885000/does-strlen-in-a-strncmp-expression-defeat-the-purpose-of-using-strncmp-ov
>>
>> ------------------------------------------------------------------------------
>> 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.
>>
>> ------------------------------------------------------------------------------
>>
>
More information about the hotspot-gc-dev
mailing list