Runtime command-line deprecation (Was Re: RFR: 8066821(S) Enhance command line processing to manage deprecating and obsoleting -XX command line arguments
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Jul 6 21:00:48 UTC 2015
Hi,
I'm happy for more clarity in the process of removing command line
arguments. I have some questions and comments below.
On 6/26/15 4:09 PM, Karen Kinnear wrote:
> 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)
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.
>
> 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
Yes, this reflects our current model.
>
> 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?
So this is my question which is around the code review in question.
Note, Derek, can you resend the RFR to hotspot-dev at openjdk.java.net so
it gets to all of us (even the compiler team may want to share in the
mechanism).
Why are UseParNewGC and MaxGCMinorPauseMillis deprecated and handled and
not deprecated and obsolete? I don't actually see why have the
distinction here?
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.
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.
>
> 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?
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?
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.
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.
thanks,
Coleen
>
> 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-runtime-dev
mailing list