RFR: 8139765: set_numeric_flag can call Flag::find_flag to determine the flag type (original) (raw)

Dmitry Dmitriev dmitry.dmitriev at oracle.com
Tue Oct 27 08:38:07 UTC 2015

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 setnumericflag function in arguments.cpp module. Current implementation of this function not determine the flag type and delegate this to the CommandLineFlags::AtPut methods. If method succeed, then flag was processed. Otherwise try next type and so on. The bad thing that CommandLineFlags::AtPut calls Flag::findflag to find the flag and check his type. Thus, for sizet flag this operation will be repeated 6 times until appropriate CommandLineFlags::AtPut found the flag. In this enhancement I add CommandLineFlags::AtPut which accepts 'Flag*' argument. Other CommandLineFlags::AtPut now calls method with 'Flag*' argument. In this case duplicated code was deleted for CommandLineFlags::AtPut and CommandLineFlagsEx::AtPut methods. New method for ccstrAtPut was not added because it not needed and CommandLineFlags and CommandLineFlagsEx have slightly different logic. And finally setnumericflag function was modified to use CommandLineFlags::AtPut with 'Flag*' argument. First of all setnumericflag looking for the flag by Flag::findflag function. And then determine the flag type and call appropriate method. In this case we call Flag::findflag 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

More information about the hotspot-runtime-dev mailing list