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 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/79feb3b0de83932c6cbf761d70051330db4d07f7/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
> disc...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-discuss

_______________________________________________
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss

Reply via email to