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

[Dmitry Dmitriev]

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