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

[gerard ziemski]

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
>