Thanks Ben,
On Sat, Feb 3, 2018 at 12:16 AM, Ben Pfaff <[email protected]> wrote: > Nice finding! > > I don't think it's necessary to inline this into the header file to get > the speedup, since its caller along the uuid_from_string() call chain is > in the same file as hexit_value(). > > I did this because hexit_value() is called also from scan_int(), which is called by ovs_scan(). The former was called around > 20K times in my run. But it's also in util.c so that's fine :) > I sent out a patch that does something similar: > https://patchwork.ozlabs.org/patch/868826/ > > On Fri, Feb 02, 2018 at 07:24:59PM +0100, Daniel Alvarez Sanchez wrote: > > Hi folks, > > > > While running rally in OpenStack we found out that ovn-northd was > > at 100% CPU most of the time. It doesn't have to be necessarily > > a problem but I wanted to do a simple profiling by running a rally task > > which creates a network (Logical Switch) and creates 6 ports on it, > > repeating the whole operation 1000 times. The ports are networks > > are also deleted. > > > > I used master branch and compiled it with -O1: > > > > CFLAGS="-O1 -g" ./configure --prefix=/usr/local/ > > --with-linux=/usr/lib/modules/`ls /usr/lib/modules/ | tail -n 1`/build > > > > gcc version 4.8.5 20150623 (Red Hat 4.8.5-16) (GCC) > > > > What I saw is that ~ 15% of the execution time was spent in > > uuid_from_string() function in util/uuid.c module which calls > hexits_value() > > which ends up calling hexit_value(). This last function gets called >1M > > times. > > > > I thought that it was worth to inline hexit_value() and use a lookup > table > > instead the switch/case [0] so I did it (find the patch below) and the > gain > > is > > that instead of a 14%, uuid_from_string() now takes a 9% of the total > > execution time. See [1]. > > > > [0] > > https://github.com/openvswitch/ovs/blob/79feb3b0de83932c6cbf761d700513 > 30db4d07f7/lib/util.c#L844 > > [1] https://imgur.com/a/3gzDF > > > > Patch: > > Note that we could make the table smaller to optimize the data cache > usage > > but then we would have to accommodate the argument and include extra > > checks. > > > > > > ------------------------------------------------------- > > > > diff --git a/lib/util.c b/lib/util.c > > index a4d22df0c..a24472690 100644 > > --- a/lib/util.c > > +++ b/lib/util.c > > @@ -839,38 +839,6 @@ str_to_double(const char *s, double *d) > > } > > } > > > > -/* Returns the value of 'c' as a hexadecimal digit. */ > > -int > > -hexit_value(int c) > > -{ > > - switch (c) { > > - case '0': case '1': case '2': case '3': case '4': > > - case '5': case '6': case '7': case '8': case '9': > > - return c - '0'; > > - > > - case 'a': case 'A': > > - return 0xa; > > - > > - case 'b': case 'B': > > - return 0xb; > > - > > - case 'c': case 'C': > > - return 0xc; > > - > > - case 'd': case 'D': > > - return 0xd; > > - > > - case 'e': case 'E': > > - return 0xe; > > - > > - case 'f': case 'F': > > - return 0xf; > > - > > - default: > > - return -1; > > - } > > -} > > - > > /* Returns the integer value of the 'n' hexadecimal digits starting at > > 's', or > > * UINTMAX_MAX if one of those "digits" is not really a hex digit. Sets > > '*ok' > > * to true if the conversion succeeds or to false if a non-hex digit is > > diff --git a/lib/util.h b/lib/util.h > > index b6639b8b8..f41e2a030 100644 > > --- a/lib/util.h > > +++ b/lib/util.h > > @@ -217,7 +217,28 @@ bool ovs_scan_len(const char *s, int *n, const char > > *format, ...); > > > > bool str_to_double(const char *, double *); > > > > -int hexit_value(int c); > > + > > +static const char hextable[] = { > > + -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, > -1,-1,-1, > > + -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, > -1,-1,-1, > > + -1,-1, 0,1,2,3,4,5,6,7,8,9,-1,-1,-1,-1,-1,-1,-1,10,11,12,13,14,15,- > 1, > > + -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, > -1,-1,-1, > > + -1,-1,10,11,12,13,14,15,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, > -1,-1,-1, > > + -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, > -1,-1,-1, > > + -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, > -1,-1,-1, > > + -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, > -1,-1,-1, > > + -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, > -1,-1,-1, > > + -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, > -1,-1,-1, > > + -1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1,-1, > -1,-1,-1 > > +}; > > + > > +/* Returns the value of 'c' as a hexadecimal digit. */ > > +static inline int > > +hexit_value(int c) > > +{ > > + return hextable[c & 0xff]; > > +} > > + > > uintmax_t hexits_value(const char *s, size_t n, bool *ok); > > > > int parse_int_string(const char *s, uint8_t *valuep, int field_width, > > > > > > ------------------------------------------------------- > > > _______________________________________________ > > discuss mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-discuss > >
_______________________________________________ discuss mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
