[systemd-devel] [PATCH, v3 3/3] find_symlinks: adds a cache of enabled unit symbolic link state

[Lennart Poettering]

On Tue, 21.10.14 15:21, Ken Sedgwick (ksedgwic at bonsai.com) wrote:

> The current find_symlinks_fd code traverses the config directories
> duplicatively. This is a performance problem if 1000s of units are
> being controlled. This patch adds a hashmap cache of symbolic link
> state which is filled in one traversal and then consulted as needed to
> prevent re-traversal.
> 
> The enabled_context cache lives in the manager struct.  Initially the
> cache is empty and is filled on first use.  A pointer to the cache is
> passed to all routines which can call find_symlinks_fd.  If a NULL is
> passed to find_symlinks_fd a temporary cache is created and used for
> the single call.

Currently, the enable/disable does not cache anything, hence every bus
call for it starts from zero and reads everything. I must say I really
like that behaviour.

With your patch you generate a system-wide cache for that, but when do
you flush it precisely? What's the logic there?

Do we really need to keep the cache around? I mean, it see that it
can help if people invoke multiple bus calls one after the other, but
then again, we actually allow passing an array in anyway, so if people
want to take benefit of the optimization they can just issue one call
with multipls args instead of many calls with one each?

I presume the cache will not consumer too much memory, but also not a
trivial amount. Don't we want to make sure we flush this out soonishly
anyway?

> The cache has two levels, the first is keyed by config_path and the
> second by the names and paths of the units.
> 
> The cache contains both forward and reverse mappings; from symbolic
> link name to target and from target to symbolic link name.
> 
> The cache contains entries for both the basename of the unit and the
> full path of the link name and target.
> 
> The test-enabled patch (previously submitted) checks that the results
> of unit_file_get_state and unit_file_get_list do not change as a
> result of this cache. This patch presumes the test-enabled patch has
> already been applied.
> 
> The test-manyunits patch (previously submitted) checks that the
> performance of unit_file_get_list is acceptable in the face of
> thousands of units.  This patch presumes the test-manyunits patch has
> already been applied.

> diff --git a/src/core/manager.c b/src/core/manager.c
> index 1a5d252..1eb9683 100644
> --- a/src/core/manager.c
> +++ b/src/core/manager.c
> @@ -464,6 +464,10 @@ int manager_new(SystemdRunningAs running_as, bool test_run, Manager **_m) {
>          if (r < 0)
>                  goto fail;
>  
> +        m->enabled = enabled_context_new();
> +        if (!m->enabled)
> +                goto fail;
> +

These days, if there's a chance that we will not need a data structure
anyway we try to allocate them the first time we need them, not during
start-up. In many cases we'll probably not require the cache at all,
hence I think this would be a really good candidate for allocating the
cache on-demand...

(That all under the condition we really really want to keep the cache
around all the time, anyway, see above)

> +void enabled_context_free(EnabledContext *ec) {
> +        char *key;
> +        Hashmap *h;
> +
> +        if (!ec)
> +                return;
> +
> +        while ((key = hashmap_first_key(ec->config_paths_forward))) {
> +                h = hashmap_steal_first(ec->config_paths_forward);
> +                hashmap_free_free_free(h);
> +                free(key);
> +        }
> +        hashmap_free_free_free(ec->config_paths_forward);
> +        ec->config_paths_forward = NULL;

ot that it would really matter, but if we removed all entries anyway
we can free the hashmap with just hasmap_free(), no need to invoke
hashmap_free_free_free()...

Sounds unnecessary to reset the hashmap to NULL here, after all, we
free the whole struct right afterwards anyway...

> +static int fill_enabled_context(
>                  int fd,
>                  const char *path,
>                  const char *config_path,
> -                bool *same_name_link) {
> -
> +                EnabledContext *ec) {
>          int r = 0;
>          _cleanup_closedir_ DIR *d = NULL;
> +        Hashmap *config_path_forward;
> +        Hashmap *config_path_reverse;
>  
> -        assert(name);
>          assert(fd >= 0);
>          assert(path);
>          assert(config_path);
> -        assert(same_name_link);
> +        assert(ec);
> +
> +        config_path_forward = hashmap_get(ec->config_paths_forward, config_path);
> +        config_path_reverse = hashmap_get(ec->config_paths_reverse, config_path);
> +
> +        /* If config_path_forward isn't cached, generate both directions */
> +        if (config_path_forward == NULL) {
> +                /* Initialize empty forward and reverse lookups for the
> +                 * enabled context cache */
> +                r = hashmap_ensure_allocated(&config_path_forward, &string_hash_ops);
> +                if (r < 0) {
> +                        return r;
> +                }

No {} around single-line if blocks please.

> +                r = hashmap_put(ec->config_paths_forward, strdup(config_path), config_path_forward);
> +                if (r < 0)
> +                        return r;
> +                r = hashmap_ensure_allocated(&config_path_reverse, &string_hash_ops);
> +                if (r < 0) {
> +                        return r;
> +                }

same here...

> +                r = hashmap_put(ec->config_paths_reverse, strdup(config_path), config_path_reverse);
> +                if (r < 0)
> +                        return r;
> +        }

This feels as if there's some rollback necessary, to ensure we don't
leave half-installed entries around in case of OOM?


> +        assert(name);
> +        assert(fd >= 0);
> +        assert(path);
> +        assert(config_path);
> +        assert(same_name_link);
> +
> +        /* If no ec is passed in, use a temporary one that auto-frees */
> +        if (ec == NULL) {
> +                ec = enabled_context_new();
> +                if (ec == NULL)
> +                        return -ENOMEM;
> +                temp_ec = ec;
> +        }

Nitpicking: we usually just check "if (ptr)" instead of "if (ptr ==
NULL)", but only for pointers and bools.

> +        config_path_forward = hashmap_get(ec->config_paths_forward, config_path);
> +
> +        /* If config_path_forward isn't cached, generate both directions */
> +        if (config_path_forward == NULL) {
> +                r = fill_enabled_context(fd, path, config_path, ec);
> +                if (r < 0)
> +                        return r;
> +        } else {
> +                safe_close(fd);
> +        }

Looks good otherwise!

Lennart

-- 
Lennart Poettering, Red Hat