core: store unit aliases in a separate set

We allocated the names set for each unit, but in the majority of cases, we'd
put only one name in the set:

$ systemctl show --value -p Names '*'|grep .|grep -v ' '|wc -l
564
$ systemctl show --value -p Names '*'|grep .|grep ' '|wc -l
16

So let's add a separate .id field, and only store aliases in the set, and only
create the set if there's at least one alias. This requires a bit of gymnastics
in the code, but I think this optimization is worth the trouble, because we
save one object for many loaded units.

In particular set_complete_move() wasn't very useful because the target
unit would always have at least one name defined, i.e. the optimization to
move the whole set over would never fire.

src/core/dbus-unit.c

@@ -102,20 +102,24 @@ static int property_get_names(
                 void *userdata,
                 sd_bus_error *error) {
 
-        Set **s = userdata;
+        Unit *u = userdata;
         Iterator i;
         const char *t;
         int r;
 
         assert(bus);
         assert(reply);
-        assert(s);
+        assert(u);
 
         r = sd_bus_message_open_container(reply, 'a', "s");
         if (r < 0)
                 return r;
 
-        SET_FOREACH(t, *s, i) {
+        r = sd_bus_message_append(reply, "s", u->id);
+        if (r < 0)
+                return r;
+
+        SET_FOREACH(t, u->aliases, i) {
                 r = sd_bus_message_append(reply, "s", t);
                 if (r < 0)
                         return r;
@@ -841,7 +845,7 @@ const sd_bus_vtable bus_unit_vtable[] = {
         SD_BUS_VTABLE_START(0),
 
         SD_BUS_PROPERTY("Id", "s", NULL, offsetof(Unit, id), SD_BUS_VTABLE_PROPERTY_CONST),
-        SD_BUS_PROPERTY("Names", "as", property_get_names, offsetof(Unit, names), SD_BUS_VTABLE_PROPERTY_CONST),
+        SD_BUS_PROPERTY("Names", "as", property_get_names, 0, SD_BUS_VTABLE_PROPERTY_CONST),
         SD_BUS_PROPERTY("Following", "s", property_get_following, 0, 0),
         SD_BUS_PROPERTY("Requires", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_REQUIRES]), SD_BUS_VTABLE_PROPERTY_CONST),
         SD_BUS_PROPERTY("Requisite", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_REQUISITE]), SD_BUS_VTABLE_PROPERTY_CONST),

src/core/load-dropin.c

@@ -19,9 +19,8 @@ static int process_deps(Unit *u, UnitDependency dependency, const char *dir_suff
         r = unit_file_find_dropin_paths(NULL,
                                         u->manager->lookup_paths.search_path,
                                         u->manager->unit_path_cache,
-                                        dir_suffix,
-                                        NULL,
-                                        u->names,
+                                        dir_suffix, NULL,
+                                        u->id, u->aliases,
                                         &paths);
         if (r < 0)
                 return r;

src/core/load-dropin.h

@@ -13,7 +13,7 @@ static inline int unit_find_dropin_paths(Unit *u, char ***paths) {
                                            u->manager->lookup_paths.search_path,
                                            u->manager->unit_path_cache,
                                            ".d", ".conf",
-                                           u->names,
+                                           u->id, u->aliases,
                                            paths);
 }
 

src/core/unit.c

@@ -93,10 +93,6 @@ Unit *unit_new(Manager *m, size_t size) {
         if (!u)
                 return NULL;
 
-        u->names = set_new(&string_hash_ops);
-        if (!u->names)
-                return mfree(u);
-
         u->manager = m;
         u->type = _UNIT_TYPE_INVALID;
         u->default_dependencies = true;
@@ -152,7 +148,8 @@ bool unit_has_name(const Unit *u, const char *name) {
         assert(u);
         assert(name);
 
-        return set_contains(u->names, (char*) name);
+        return streq_ptr(name, u->id) ||
+               set_contains(u->aliases, name);
 }
 
 static void unit_init(Unit *u) {
@@ -207,8 +204,25 @@ static void unit_init(Unit *u) {
                 UNIT_VTABLE(u)->init(u);
 }
 
+static int unit_add_alias(Unit *u, char *donated_name) {
+        int r;
+
+        /* Make sure that u->names is allocated. We may leave u->names
+         * empty if we fail later, but this is not a problem. */
+        r = set_ensure_allocated(&u->aliases, &string_hash_ops);
+        if (r < 0)
+                return r;
+
+        r = set_put(u->aliases, donated_name);
+        if (r < 0)
+                return r;
+        assert(r > 0);
+
+        return 0;
+}
+
 int unit_add_name(Unit *u, const char *text) {
-        _cleanup_free_ char *s = NULL, *i = NULL;
+        _cleanup_free_ char *name = NULL, *instance = NULL;
         UnitType t;
         int r;
 
@@ -216,99 +230,101 @@ int unit_add_name(Unit *u, const char *text) {
         assert(text);
 
         if (unit_name_is_valid(text, UNIT_NAME_TEMPLATE)) {
-
                 if (!u->instance)
                         return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL),
                                                     "instance is not set when adding name '%s': %m", text);
 
-                r = unit_name_replace_instance(text, u->instance, &s);
+                r = unit_name_replace_instance(text, u->instance, &name);
                 if (r < 0)
                         return log_unit_debug_errno(u, r,
                                                     "failed to build instance name from '%s': %m", text);
         } else {
-                s = strdup(text);
-                if (!s)
+                name = strdup(text);
+                if (!name)
                         return -ENOMEM;
         }
 
-        if (set_contains(u->names, s))
+        if (unit_has_name(u, name))
                 return 0;
-        if (hashmap_contains(u->manager->units, s))
+
+        if (hashmap_contains(u->manager->units, name))
                 return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EEXIST),
-                                            "unit already exist when adding name '%s': %m", text);
+                                            "unit already exist when adding name '%s': %m", name);
 
-        if (!unit_name_is_valid(s, UNIT_NAME_PLAIN|UNIT_NAME_INSTANCE))
+        if (!unit_name_is_valid(name, UNIT_NAME_PLAIN|UNIT_NAME_INSTANCE))
                 return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL),
-                                            "name '%s' is invalid: %m", text);
+                                            "name '%s' is invalid: %m", name);
 
-        t = unit_name_to_type(s);
+        t = unit_name_to_type(name);
         if (t < 0)
                 return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL),
-                                            "failed to to derive unit type from name '%s': %m", text);
+                                            "failed to to derive unit type from name '%s': %m", name);
 
         if (u->type != _UNIT_TYPE_INVALID && t != u->type)
                 return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL),
                                             "unit type is illegal: u->type(%d) and t(%d) for name '%s': %m",
-                                            u->type, t, text);
+                                            u->type, t, name);
 
-        r = unit_name_to_instance(s, &i);
+        r = unit_name_to_instance(name, &instance);
         if (r < 0)
-                return log_unit_debug_errno(u, r, "failed to extract instance from name '%s': %m", text);
+                return log_unit_debug_errno(u, r, "failed to extract instance from name '%s': %m", name);
 
-        if (i && !unit_type_may_template(t))
-                return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), "templates are not allowed for name '%s': %m", text);
+        if (instance && !unit_type_may_template(t))
+                return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), "templates are not allowed for name '%s': %m", name);
 
         /* Ensure that this unit is either instanced or not instanced,
          * but not both. Note that we do allow names with different
          * instance names however! */
-        if (u->type != _UNIT_TYPE_INVALID && !u->instance != !i)
+        if (u->type != _UNIT_TYPE_INVALID && !!u->instance != !!instance)
                 return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL),
                                             "instance is illegal: u->type(%d), u->instance(%s) and i(%s) for name '%s': %m",
-                                            u->type, u->instance, i, text);
+                                            u->type, u->instance, instance, name);
 
-        if (!unit_type_may_alias(t) && !set_isempty(u->names))
-                return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EEXIST), "symlinks are not allowed for name '%s': %m", text);
+        if (u->id && !unit_type_may_alias(t))
+                return log_unit_debug_errno(u, SYNTHETIC_ERRNO(EEXIST), "aliases are not allowed for name '%s': %m", name);
 
         if (hashmap_size(u->manager->units) >= MANAGER_MAX_NAMES)
                 return log_unit_debug_errno(u, SYNTHETIC_ERRNO(E2BIG), "too many units: %m");
 
-        r = set_put(u->names, s);
+        /* Add name to the global hashmap first, because that's easier to undo */
+        r = hashmap_put(u->manager->units, name, u);
         if (r < 0)
-                return r;
-        assert(r > 0);
-
-        r = hashmap_put(u->manager->units, s, u);
-        if (r < 0) {
-                (void) set_remove(u->names, s);
                 return log_unit_debug_errno(u, r, "add unit to hashmap failed for name '%s': %m", text);
-        }
 
-        if (u->type == _UNIT_TYPE_INVALID) {
+        if (u->id) {
+                r = unit_add_alias(u, name); /* unit_add_alias() takes ownership of the name on success */
+                if (r < 0) {
+                        hashmap_remove(u->manager->units, name);
+                        return r;
+                }
+                TAKE_PTR(name);
+
+        } else {
+                /* A new name, we don't need the set yet. */
+                assert(u->type == _UNIT_TYPE_INVALID);
+                assert(!u->instance);
+
                 u->type = t;
-                u->id = s;
-                u->instance = TAKE_PTR(i);
+                u->id = TAKE_PTR(name);
+                u->instance = TAKE_PTR(instance);
 
                 LIST_PREPEND(units_by_type, u->manager->units_by_type[t], u);
-
                 unit_init(u);
         }
 
-        s = NULL;
-
         unit_add_to_dbus_queue(u);
         return 0;
 }
 
 int unit_choose_id(Unit *u, const char *name) {
-        _cleanup_free_ char *t = NULL;
-        char *s, *i;
+        _cleanup_free_ char *t = NULL, *i = NULL;
+        char *s;
         int r;
 
         assert(u);
         assert(name);
 
         if (unit_name_is_valid(name, UNIT_NAME_TEMPLATE)) {
-
                 if (!u->instance)
                         return -EINVAL;
 
@@ -319,17 +335,27 @@ int unit_choose_id(Unit *u, const char *name) {
                 name = t;
         }
 
-        /* Selects one of the names of this unit as the id */
-        s = set_get(u->names, (char*) name);
+        if (streq_ptr(u->id, name))
+                return 0; /* Nothing to do. */
+
+        /* Selects one of the aliases of this unit as the id */
+        s = set_get(u->aliases, (char*) name);
         if (!s)
                 return -ENOENT;
 
         /* Determine the new instance from the new id */
-        r = unit_name_to_instance(s, &i);
+        r = unit_name_to_instance(name, &i);
         if (r < 0)
                 return r;
 
-        u->id = s;
+        if (u->id) {
+                r = set_remove_and_put(u->aliases, name, u->id);
+                if (r < 0)
+                        return r;
+        } else
+                assert_se(set_remove(u->aliases, name)); /* see set_get() above… */
+
+        u->id = s; /* Old u->id is now stored in the set, and s is not stored anywhere */
 
         free(u->instance);
         u->instance = i;
@@ -632,8 +658,10 @@ void unit_free(Unit *u) {
 
         unit_free_requires_mounts_for(u);
 
-        SET_FOREACH(t, u->names, i)
+        SET_FOREACH(t, u->aliases, i)
                 hashmap_remove_value(u->manager->units, t, u);
+        if (u->id)
+                hashmap_remove_value(u->manager->units, u->id, u);
 
         if (!sd_id128_is_null(u->invocation_id))
                 hashmap_remove_value(u->manager->units_by_invocation_id, &u->invocation_id, u);
@@ -730,11 +758,11 @@ void unit_free(Unit *u) {
         free(u->instance);
 
         free(u->job_timeout_reboot_arg);
-
-        set_free_free(u->names);
-
         free(u->reboot_arg);
 
+        set_free_free(u->aliases);
+        free(u->id);
+
         free(u);
 }
 
@@ -789,21 +817,6 @@ const char* unit_sub_state_to_string(Unit *u) {
         return UNIT_VTABLE(u)->sub_state_to_string(u);
 }
 
-static int set_complete_move(Set **s, Set **other) {
-        assert(s);
-        assert(other);
-
-        if (!other)
-                return 0;
-
-        if (*s)
-                return set_move(*s, *other);
-        else
-                *s = TAKE_PTR(*other);
-
-        return 0;
-}
-
 static int hashmap_complete_move(Hashmap **s, Hashmap **other) {
         assert(s);
         assert(other);
@@ -820,23 +833,28 @@ static int hashmap_complete_move(Hashmap **s, Hashmap **other) {
 }
 
 static int merge_names(Unit *u, Unit *other) {
-        char *t;
+        char *name;
         Iterator i;
         int r;
 
         assert(u);
         assert(other);
 
-        r = set_complete_move(&u->names, &other->names);
+        r = unit_add_alias(u, other->id);
         if (r < 0)
                 return r;
 
-        set_free_free(other->names);
-        other->names = NULL;
-        other->id = NULL;
+        r = set_move(u->aliases, other->aliases);
+        if (r < 0) {
+                set_remove(u->aliases, other->id);
+                return r;
+        }
 
-        SET_FOREACH(t, u->names, i)
-                assert_se(hashmap_replace(u->manager->units, t, u) == 0);
+        TAKE_PTR(other->id);
+        other->aliases = set_free_free(other->aliases);
+
+        SET_FOREACH(name, u->aliases, i)
+                assert_se(hashmap_replace(u->manager->units, name, u) == 0);
 
         return 0;
 }
@@ -1241,9 +1259,8 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) {
                 "%s-> Unit %s:\n",
                 prefix, u->id);
 
-        SET_FOREACH(t, u->names, i)
-                if (!streq(t, u->id))
-                        fprintf(f, "%s\tAlias: %s\n", prefix, t);
+        SET_FOREACH(t, u->aliases, i)
+                fprintf(f, "%s\tAlias: %s\n", prefix, t);
 
         fprintf(f,
                 "%s\tDescription: %s\n"

src/core/unit.h

@@ -117,10 +117,10 @@ typedef struct Unit {
         FreezerState freezer_state;
         sd_bus_message *pending_freezer_message;
 
-        char *id; /* One name is special because we use it for identification. Points to an entry in the names set */
+        char *id;   /* The one special name that we use for identification */
         char *instance;
 
-        Set *names;
+        Set *aliases; /* All the other names. */
 
         /* For each dependency type we maintain a Hashmap whose key is the Unit* object, and the value encodes why the
          * dependency exists, using the UnitDependencyInfo type */

src/shared/dropin.c

@@ -226,30 +226,35 @@ int unit_file_find_dropin_paths(
                 Set *unit_path_cache,
                 const char *dir_suffix,
                 const char *file_suffix,
-                const Set *names,
+                const char *name,
+                const Set *aliases,
                 char ***ret) {
 
         _cleanup_strv_free_ char **dirs = NULL;
-        char *name, **p;
+        const char *n;
+        char **p;
         Iterator i;
         int r;
 
         assert(ret);
 
-        SET_FOREACH(name, names, i)
+        if (name)
                 STRV_FOREACH(p, lookup_path)
                         (void) unit_file_find_dirs(original_root, unit_path_cache, *p, name, dir_suffix, &dirs);
 
+        SET_FOREACH(n, aliases, i)
+                STRV_FOREACH(p, lookup_path)
+                        (void) unit_file_find_dirs(original_root, unit_path_cache, *p, n, dir_suffix, &dirs);
+
         /* All the names in the unit are of the same type so just grab one. */
-        name = (char*) set_first(names);
-        if (name) {
+        n = name ?: (const char*) set_first(aliases);
+        if (n) {
                 UnitType type = _UNIT_TYPE_INVALID;
 
-                type = unit_name_to_type(name);
+                type = unit_name_to_type(n);
                 if (type < 0)
                         return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
-                                               "Failed to to derive unit type from unit name: %s",
-                                               name);
+                                               "Failed to to derive unit type from unit name: %s", n);
 
                 /* Special top level drop in for "<unit type>.<suffix>". Add this last as it's the most generic
                  * and should be able to be overridden by more specific drop-ins. */

src/shared/dropin.h

@@ -21,5 +21,6 @@ int unit_file_find_dropin_paths(
                 Set *unit_path_cache,
                 const char *dir_suffix,
                 const char *file_suffix,
-                const Set *names,
+                const char *name,
+                const Set *aliases,
                 char ***paths);

src/systemctl/systemctl.c

@@ -2673,7 +2673,7 @@ static int unit_find_paths(
                 if (ret_dropin_paths) {
                         r = unit_file_find_dropin_paths(arg_root, lp->search_path, NULL,
                                                         ".d", ".conf",
-                                                        names, &dropins);
+                                                        NULL, names, &dropins);
                         if (r < 0)
                                 return r;
                 }