[systemd-devel] [PATCH] util: Use a shared lookup function for string tables

[David Herrmann]

Hi

On Fri, Feb 13, 2015 at 9:40 PM, Bruno Bottazzini
<bruno.bottazzini at intel.com> wrote:
> Macro DEFINE_STRING_TABLE_LOOKUP expands to a new function for each
> of the almost 120 tables throghout the code.
> Move the its implementation to a function (guaranteed to never be inlined),
> and make the macro expand to an inlined function that calls this function.
> This saves a few kilobytes from the systemd binary
> ---
>  src/shared/util.c |  9 +++++++++
>  src/shared/util.h | 15 ++++++---------
>  2 files changed, 15 insertions(+), 9 deletions(-)

Hm, this adds an additional level of indirection, but -lto should
probably fix this up. Haven't verified this, though.
Only a few lookup-tables are used in fast-paths, so I guess the
.text-size reduction is worth it. I fixed up some issues (see below)
and applied this patch.

Anyway, as a followup, we might consider moving all the
DEFINE_STRING_TABLE_LOOKUP() into header-files and export the
string-tables. This way, we get rid of the double-indirection even
without relying on the compiler. And we can actually get rid of one
LOC per lookup-table.

> diff --git a/src/shared/util.c b/src/shared/util.c
> index 891182a..5c8e698 100644
> --- a/src/shared/util.c
> +++ b/src/shared/util.c
> @@ -8080,3 +8080,12 @@ int syslog_parse_priority(const char **p, int *priority, bool with_facility) {
>          *p += k;
>          return 1;
>  }
> +
> +size_t _string_table_lookup(const char *const *table, size_t len, const char *key){
> +        size_t i;
> +        if (key)
> +            for (i = 0; i < len; i++)
> +                    if (table[i] && streq(table[i], key))
> +                            return i;
> +        return (size_t) -1;

I fixed up the coding-style:
 - we avoid prefixing functions with underscores
 - several whitespace fixups
 - use streq_ptr()

Furthermore, I changed 'size_t' to 'ssize_t'. Imagine a 32bit
'size_t', but a 64bit 'type' in the lookup-macro, this will produce
2^32-1 on ENOENT, instead of 2^64-1. We need to actually return the
signed value, otherwise sign extension will not work.

Thanks
David

> +}
> diff --git a/src/shared/util.h b/src/shared/util.h
> index ca0c2e5..caa960d 100644
> --- a/src/shared/util.h
> +++ b/src/shared/util.h
> @@ -334,6 +334,8 @@ int make_console_stdio(void);
>  int dev_urandom(void *p, size_t n);
>  void random_bytes(void *p, size_t n);
>  void initialize_srand(void);
> +size_t _string_table_lookup(const char *const *table, size_t len, const char *key)
> +                            __attribute__((noinline));
>
>  static inline uint64_t random_u64(void) {
>          uint64_t u;
> @@ -355,16 +357,11 @@ static inline uint32_t random_u32(void) {
>                  return name##_table[i];                                 \
>          }
>
> +
>  #define _DEFINE_STRING_TABLE_LOOKUP_FROM_STRING(name,type,scope)        \
> -        scope type name##_from_string(const char *s) {                  \
> -                type i;                                                 \
> -                if (!s)                                                 \
> -                        return (type) -1;                               \
> -                for (i = 0; i < (type)ELEMENTSOF(name##_table); i++)    \
> -                        if (name##_table[i] &&                          \
> -                            streq(name##_table[i], s))                  \
> -                                return i;                               \
> -                return (type) -1;                                       \
> +        scope inline type name##_from_string(const char *s) {           \
> +                return (type)_string_table_lookup(name##_table,         \
> +                        ELEMENTSOF(name##_table), s);                   \
>          }
>
>  #define _DEFINE_STRING_TABLE_LOOKUP(name,type,scope)                    \
> --
> 1.9.1
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel