Runtime command-line deprecation (Was Re: RFR: 8066821(S) Enhance command line processing to manage deprecating and obsoleting -XX command line arguments

Fri Jun 26 22:33:40 UTC 2015

In line replies

On 6/26/2015 1: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)
>
> 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)

For a flag that selects the GC  the "do nothing" part would mean we would
have to pick a GC to use.   Historically (we only have one precedent for 
this)
we've avoided having to make that decision by Deprecating & Handling.
The customer gets the GC requested but also the warning.  In the next 
release
the option becomes Dead (again avoiding having to pick a replacement GC
for the customer).

>      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

Even for External flags we're done only "Deprecate & Handle" and 
"Dead".  The
two External flags selected a GC (-Xinggc and -Xconcgc).

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

Using CMS as the example, we will Deprecate CMS in 9 and intend to
remove it in 10 but that assumes that G1 is good enough to replace
CMS by 10.  CMS has some use cases where it is going to be very
hard for G1 out perform CMS.   If we get enough yelling from
customers, it could be 11 (please, please no) before CMS is Dead
(to Oracle).

Jon

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