RFR: 8139765: set_numeric_flag can call Flag::find_flag to determine the flag type

[Jiangli Zhou]

Hi Dmitry,

Looks good to me.

Thanks,
Jiangli

> On Oct 29, 2015, at 2:10 AM, Dmitry Dmitriev <dmitry.dmitriev at oracle.com> wrote:
> 
> Hello,
> 
> Still need a Reviewer please. Thanks!
> 
> Dmitry
> 
> On 23.10.2015 15:34, Dmitry Dmitriev wrote:
>> Hi Gerard,
>> 
>> Thank you for the review and provided feedback!
>> 
>> I implement #2. Also, I change line 822 in arguments.cpp from "if (!result) {" to "if (result == NULL) {".
>> 
>> About #1: I think we should leave "guarantee()" call in CommandLineFlagsEx functions. guarantee() in this case ensures that flag embedded in CommandLineFlagWithType is exist and with correct type. If guarantee failed, then we have a conceptual error, since we try to call improper CommandLineFlagsEx function, i.e. call CommandLineFlagsEx::boolAtPut for intx flag. Without guarantee we will get an error in case of bad flag type, but if the caller not check error code, then flag will remain unchanged. Thus, I think that we need to leave "guarantee()" in these APIs.
>> 
>> webrev.01: http://cr.openjdk.java.net/~ddmitriev/8139765/webrev.01/ <http://cr.openjdk.java.net/%7Eddmitriev/8139765/webrev.01/>
>> 
>> Thanks,
>> Dmitry
>> 
>> 
>> On 22.10.2015 18:55, gerard ziemski wrote:
>>> Wow, excellent idea for an optimization!
>>> 
>>> I only have 2 feedback issues:
>>> 
>>> 
>>> #1 file globals.cpp
>>> 
>>> Since we are refactoring CommandLineFlags::*AtPut() APIs in terms of one API, I think the "gurantee() call is no longer needed and is redundant?
>>> 
>>> Ex. The guarantee() checks for NULL and type (line 839), but only reports "wrong flag type" vs the API impl (lines 819,820) checks them separately and reports problem more accurately.
>>> 
>>> I believe we are better served by removing the "guarantee" call completely from those APIs - those checks are performed elsewhere.
>>> 
>>> 
>>> #2 file arguments.cpp
>>> 
>>> This might be nit picking, but since we are optimizing the code, then perhaps instead of:
>>> 
>>> if (A) {
>>> }
>>> if (B) {
>>> }
>>> if (C) {
>>> }...
>>> 
>>> we should have:
>>> 
>>> if (A) {
>>> } else if (B) {
>>> } else if (C) {
>>> }...
>>> 
>>> This way if we have A we don't have to bother checking B or C.
>>> 
>>> The compiler might optimize that code this way anyhow, but it would be nice to have this programmed explicitly.
>>> 
>>> 
>>> Very, very nice work and idea!
>>> 
>>> 
>>> cheers
>>> 
>>> On 10/21/2015 10:40 AM, Dmitry Dmitriev wrote:
>>>> Hello,
>>>> 
>>>> Please review enhancement of set_numeric_flag function in arguments.cpp module.
>>>> Current implementation of this function not determine the flag type and delegate this to the
>>>> CommandLineFlags::<type>AtPut methods. If method succeed, then flag was processed. Otherwise try next type and so on.
>>>> The bad thing that CommandLineFlags::<type>AtPut calls Flag::find_flag to find the flag and check his type. Thus, for
>>>> size_t flag this operation will be repeated 6 times until appropriate CommandLineFlags::<type>AtPut found the flag.
>>>> 
>>>> In this enhancement I add CommandLineFlags::<type>AtPut which accepts 'Flag*' argument. Other
>>>> CommandLineFlags::<type>AtPut now calls method with 'Flag*' argument. In this case duplicated code was deleted for
>>>> CommandLineFlags::<type>AtPut and CommandLineFlagsEx::<type>AtPut methods. New method for ccstrAtPut was not added
>>>> because it not needed and CommandLineFlags and CommandLineFlagsEx have slightly different logic.
>>>> 
>>>> And finally set_numeric_flag function was modified to use CommandLineFlags::<type>AtPut with 'Flag*' argument. First of
>>>> all set_numeric_flag looking for the flag by Flag::find_flag function. And then determine the flag type and call
>>>> appropriate method. In this case we call Flag::find_flag function only once.
>>>> 
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8139765
>>>> webrev.00: http://cr.openjdk.java.net/~ddmitriev/8139765/webrev.00/
>>>> <http://cr.openjdk.java.net/%7Eddmitriev/8139765/webrev.00/>
>>>> Testing: JPRT(hotspot test set), hotspot all, vm.quick
>>>> 
>>>> Thanks,
>>>> Dmitry
>>>> 
>> 
>