[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8139765: set_numeric_flag can call Flag::find_flag to determine the flag type
From:       Dmitry Dmitriev <dmitry.dmitriev () oracle ! com>
Date:       2015-10-29 21:00:59
Message-ID: 5632890B.6060609 () oracle ! com
[Download RAW message or body]

Hi Jiangli,

Thank you for the review!

Dmitry

On 29.10.2015 19:01, Jiangli Zhou wrote:
> Hi Dmitry,
> 
> Looks good to me.
> 
> Thanks,
> Jiangli
> 
> > On Oct 29, 2015, at 2:10 AM, Dmitry Dmitriev <dmitry.dmitriev@oracle.com> wrote:
> > 
> > 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
> > > > > 


[prev in list] [next in list] [prev in thread] [next in thread]