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.
>>>
>>> ------------------------------------------------------------------------------
>>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20150706/46d8bb87/attachment-0001.html>


More information about the hotspot-gc-dev mailing list