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

[Dmitry Dmitriev]

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