[systemd-devel] [PATCH, v3 2/3] tests: unit_file_get_list performance with many units

[Lennart Poettering]

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

> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/param.h>
> +#include <unistd.h>
> +#include <time.h>
> +
> +#include "manager.h"
> +#include "macro.h"
> +#include "util.h"
> +
> +static const int NUNITS = 3000;

Tss, this is not C++. ;-)

Just use #define for this...

> +
> +static const char *many_path(int unitnum) {
> +        static char path[PATH_MAX];
> +        snprintf(path, PATH_MAX, "%s/%s/many-%06d.service",
> +                 root_dir, "usr/lib/systemd/system", unitnum);
> +        return path;
> +}

This is a test, so we don't need to be that strict, but please try to
avoid allocating fixed-size strings if possible (unless you are in an
inner loop, and you know the limit is short, and it matters because of
speed...), use asprintf() instead, to allocate a dynamically sized
string. _cleanup_free_ makes it easy to get paths like that cleaned
up. 

Nitpick: we don't break lines that eagerly.

> +
> +static const char *link_path(int unitnum) {
> +        static char path[PATH_MAX];
> +        snprintf(path, PATH_MAX, "%s/%s/many-%06d.service",
> +                 root_dir, "etc/systemd/system/some.target.wants", unitnum);
> +        return path;
> +}
> +
> +static const char *another_path(void) {
> +        static char path[PATH_MAX];
> +        snprintf(path, PATH_MAX, "%s/%s/another.service",
> +                 root_dir, "usr/lib/systemd/system");
> +        return path;


Nitpick: when just concatenating paths we really prefer strjoin()
instead of asprintf()... It's waaay faster. (doesn't matter here of
course, it's a test, but just wanted to mention that...)

> +        fprintf(stderr, "testing with %d unit files\n", NUNITS);
> +
> +        t0 = time(NULL);

Please don't use time(), use now() instead and store the time in a
usec_t. We pretty universally adopted simple timekeeping with now() in
systemd, and we really should not start here with time()...

> +        h = hashmap_new(&string_hash_ops);

OOM check please!

> +        r = unit_file_get_list(UNIT_FILE_SYSTEM, root_dir, h);
> +        assert_se_cleanup(r >= 0);
> +        HASHMAP_FOREACH(p, h, i) {
> +                ++count;
> +        }

Nitpick: single line {} blocks please without the {}...

> +        fprintf(stderr, "saw %d units\n", count);

All fprintf() to stderr should really preferrably be done with
log_error() or a related call...

> +        assert_se_cleanup(count == 3015);
> +        t1 = time(NULL);
> +
> +        fprintf(stderr, "unit_file_get_list took %ld seconds\n",
> +                (long) (t1 - t0));

same here...

> +
> +        assert_se_cleanup(t1 - t0 < 10);
> +}

This looks dangerous should these tests run on some overworked build
system host.

Otherwise looks great!

Thanks,

Lennart

-- 
Lennart Poettering, Red Hat