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

[Dmitry Dmitriev]

Hi Gerard,

Thank you for the review! Yes, let's handle "guarantee" by JDK-8085866. 
Still needs a Reviewer please.

Dmitry

On 26.10.2015 21:43, gerard ziemski wrote:
> 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
>>>>
>>
>>