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

[gerard ziemski]

hi Dmitry,

I disagree on #2 - the "guarantee" there seems like a lazy solution to not checking the results of the API usage, but 
it's perhaps out of the scope for this issue and can be tackled later by JDK-8085866.

Otherwise looks good.

(r)eviewed


cheers



On 10/23/2015 07:34 AM, 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
>>>
>
>