xkb_x11_keymap_new_from_device: Less X11 round-trips

On my system, calling xkb_x11_keymap_new_from_device() did 78 round trips to the
X11 server, which seems excessive. This commit brings this number down to about
9 to 10 round trips.

The existing functions adopt_atom() and adopt_atoms() guarantee that the atom
was adopted by the time they return. Thus, each call to these functions must do
a round-trip. However, none of the callers need this guarantee.

This commit makes "atom adopting" asynchronous: Only some time later is the atom
actually adopted. Until then, it is in some pending "limbo" state.

This actually fixes a TODO in the comments.

Fixes: https://github.com/xkbcommon/libxkbcommon/issues/216
Signed-off-by: Uli Schlachter <psychon@znc.in>

src/x11/keymap.c

@@ -852,7 +852,7 @@ fail:
 }
 
 static bool
-get_type_names(struct xkb_keymap *keymap, xcb_connection_t *conn,
+get_type_names(struct xkb_keymap *keymap, struct x11_atom_interner *interner,
                xcb_xkb_get_names_reply_t *reply,
                xcb_xkb_get_names_value_list_t *list)
 {
@@ -880,13 +880,9 @@ get_type_names(struct xkb_keymap *keymap, xcb_connection_t *conn,
 
         ALLOC_OR_FAIL(type->level_names, type->num_levels);
 
-        if (!adopt_atom(keymap->ctx, conn, wire_type_name, &type->name))
+        x11_atom_interner_adopt_atom(interner, wire_type_name, &type->name);
-            goto fail;
+        x11_atom_interner_adopt_atoms(interner, kt_level_names_iter,
-
+                                     type->level_names, wire_num_levels);
-        if (!adopt_atoms(keymap->ctx, conn,
-                         kt_level_names_iter, type->level_names,
-                         wire_num_levels))
-            goto fail;
 
         type->num_level_names = type->num_levels;
         kt_level_names_iter += wire_num_levels;
@@ -901,7 +897,8 @@ fail:
 }
 
 static bool
-get_indicator_names(struct xkb_keymap *keymap, xcb_connection_t *conn,
+get_indicator_names(struct xkb_keymap *keymap,
+                    struct x11_atom_interner *interner,
                     xcb_xkb_get_names_reply_t *reply,
                     xcb_xkb_get_names_value_list_t *list)
 {
@@ -914,8 +911,7 @@ get_indicator_names(struct xkb_keymap *keymap, xcb_connection_t *conn,
             xcb_atom_t wire = *iter;
             struct xkb_led *led = &keymap->leds[i];
 
-            if (!adopt_atom(keymap->ctx, conn, wire, &led->name))
+            x11_atom_interner_adopt_atom(interner, wire, &led->name);
-                return false;
 
             iter++;
         }
@@ -928,7 +924,7 @@ fail:
 }
 
 static bool
-get_vmod_names(struct xkb_keymap *keymap, xcb_connection_t *conn,
+get_vmod_names(struct xkb_keymap *keymap, struct x11_atom_interner *interner,
                xcb_xkb_get_names_reply_t *reply,
                xcb_xkb_get_names_value_list_t *list)
 {
@@ -947,8 +943,7 @@ get_vmod_names(struct xkb_keymap *keymap, xcb_connection_t *conn,
             xcb_atom_t wire = *iter;
             struct xkb_mod *mod = &keymap->mods.mods[NUM_REAL_MODS + i];
 
-            if (!adopt_atom(keymap->ctx, conn, wire, &mod->name))
+            x11_atom_interner_adopt_atom(interner, wire, &mod->name);
-                return false;
 
             iter++;
         }
@@ -958,7 +953,7 @@ get_vmod_names(struct xkb_keymap *keymap, xcb_connection_t *conn,
 }
 
 static bool
-get_group_names(struct xkb_keymap *keymap, xcb_connection_t *conn,
+get_group_names(struct xkb_keymap *keymap, struct x11_atom_interner *interner,
                 xcb_xkb_get_names_reply_t *reply,
                 xcb_xkb_get_names_value_list_t *list)
 {
@@ -968,9 +963,7 @@ get_group_names(struct xkb_keymap *keymap, xcb_connection_t *conn,
     keymap->num_group_names = msb_pos(reply->groupNames);
     ALLOC_OR_FAIL(keymap->group_names, keymap->num_group_names);
 
-    if (!adopt_atoms(keymap->ctx, conn,
+    x11_atom_interner_adopt_atoms(interner, iter, keymap->group_names, length);
-                     iter, keymap->group_names, length))
-        goto fail;
 
     return true;
 
@@ -1051,7 +1044,7 @@ fail:
 }
 
 static bool
-get_names(struct xkb_keymap *keymap, xcb_connection_t *conn,
+get_names(struct xkb_keymap *keymap, struct x11_atom_interner *interner,
           uint16_t device_id)
 {
     static const xcb_xkb_name_detail_t wanted =
@@ -1072,6 +1065,7 @@ get_names(struct xkb_keymap *keymap, xcb_connection_t *conn,
          XCB_XKB_NAME_DETAIL_KEY_NAMES |
          XCB_XKB_NAME_DETAIL_VIRTUAL_MOD_NAMES);
 
+    xcb_connection_t *conn = interner->conn;
     xcb_xkb_get_names_cookie_t cookie =
         xcb_xkb_get_names(conn, device_id, wanted);
     xcb_xkb_get_names_reply_t *reply =
@@ -1097,10 +1091,10 @@ get_names(struct xkb_keymap *keymap, xcb_connection_t *conn,
         !get_atom_name(conn, list.symbolsName, &keymap->symbols_section_name) ||
         !get_atom_name(conn, list.typesName, &keymap->types_section_name) ||
         !get_atom_name(conn, list.compatName, &keymap->compat_section_name) ||
-        !get_type_names(keymap, conn, reply, &list) ||
+        !get_type_names(keymap, interner, reply, &list) ||
-        !get_indicator_names(keymap, conn, reply, &list) ||
+        !get_indicator_names(keymap, interner, reply, &list) ||
-        !get_vmod_names(keymap, conn, reply, &list) ||
+        !get_vmod_names(keymap, interner, reply, &list) ||
-        !get_group_names(keymap, conn, reply, &list) ||
+        !get_group_names(keymap, interner, reply, &list) ||
         !get_key_names(keymap, conn, reply, &list) ||
         !get_aliases(keymap, conn, reply, &list))
         goto fail;
@@ -1169,14 +1163,23 @@ xkb_x11_keymap_new_from_device(struct xkb_context *ctx,
     if (!keymap)
         return NULL;
 
+    struct x11_atom_interner interner;
+    x11_atom_interner_init(&interner, ctx, conn);
+
     if (!get_map(keymap, conn, device_id) ||
         !get_indicator_map(keymap, conn, device_id) ||
         !get_compat_map(keymap, conn, device_id) ||
-        !get_names(keymap, conn, device_id) ||
+        !get_names(keymap, &interner, device_id) ||
         !get_controls(keymap, conn, device_id)) {
         xkb_keymap_unref(keymap);
         return NULL;
     }
 
+    x11_atom_interner_round_trip(&interner);
+    if (interner.had_error) {
+        xkb_keymap_unref(keymap);
+        return NULL;
+    }
+
     return keymap;
 }

src/x11/util.c

@@ -169,14 +169,9 @@ struct x11_atom_cache {
     size_t len;
 };
 
-bool
+static struct x11_atom_cache *
-adopt_atoms(struct xkb_context *ctx, xcb_connection_t *conn,
+get_cache(struct xkb_context *ctx, xcb_connection_t *conn)
-            const xcb_atom_t *from, xkb_atom_t *to, const size_t count)
 {
-    enum { SIZE = 128 };
-    xcb_get_atom_name_cookie_t cookies[SIZE];
-    const size_t num_batches = ROUNDUP(count, SIZE) / SIZE;
-
     if (!ctx->x11_atom_cache) {
         ctx->x11_atom_cache = calloc(1, sizeof(struct x11_atom_cache));
     }
@@ -186,79 +181,112 @@ adopt_atoms(struct xkb_context *ctx, xcb_connection_t *conn,
         cache->conn = conn;
         cache->len = 0;
     }
+    return cache;
+}
 
-    memset(to, 0, count * sizeof(*to));
+void
+x11_atom_interner_init(struct x11_atom_interner *interner,
+                       struct xkb_context *ctx, xcb_connection_t *conn)
+{
+    interner->had_error = false;
+    interner->ctx = ctx;
+    interner->conn = conn;
+    interner->num_pending = 0;
+    interner->num_copies = 0;
+}
 
-    /* Send and collect the atoms in batches of reasonable SIZE. */
+void
-    for (size_t batch = 0; batch < num_batches; batch++) {
+x11_atom_interner_adopt_atom(struct x11_atom_interner *interner,
-        const size_t start = batch * SIZE;
+                             const xcb_atom_t atom, xkb_atom_t *out)
-        const size_t stop = MIN((batch + 1) * SIZE, count);
+{
+    *out = 0;
 
-        /* Send. */
+    /* Can be NULL in case the malloc failed. */
-        for (size_t i = start; i < stop; i++) {
+    struct x11_atom_cache *cache = get_cache(interner->ctx, interner->conn);
-            bool cache_hit = false;
+
-            if (cache) {
+retry:
-                for (size_t c = 0; c < cache->len; c++) {
+
-                    if (cache->cache[c].from == from[i]) {
+    /* Already in the cache? */
-                        to[i] = cache->cache[c].to;
+    if (cache) {
-                        cache_hit = true;
+        for (size_t c = 0; c < cache->len; c++) {
-                        break;
+            if (cache->cache[c].from == atom) {
-                    }
+                *out = cache->cache[c].to;
-                }
+                return;
             }
-            if (!cache_hit && from[i] != XCB_ATOM_NONE)
-                cookies[i % SIZE] = xcb_get_atom_name(conn, from[i]);
-        }
-
-        /* Collect. */
-        for (size_t i = start; i < stop; i++) {
-            xcb_get_atom_name_reply_t *reply;
-
-            if (from[i] == XCB_ATOM_NONE)
-                continue;
-
-            /* Was filled from cache. */
-            if (to[i] != 0)
-                continue;
-
-            reply = xcb_get_atom_name_reply(conn, cookies[i % SIZE], NULL);
-            if (!reply)
-                goto err_discard;
-
-            to[i] = xkb_atom_intern(ctx,
-                                    xcb_get_atom_name_name(reply),
-                                    xcb_get_atom_name_name_length(reply));
-            free(reply);
-
-            if (to[i] == XKB_ATOM_NONE)
-                goto err_discard;
-
-            if (cache && cache->len < ARRAY_SIZE(cache->cache)) {
-                size_t idx = cache->len++;
-                cache->cache[idx].from = from[i];
-                cache->cache[idx].to = to[i];
-            }
-
-            continue;
-
-            /*
-             * If we don't discard the uncollected replies, they just
-             * sit in the XCB queue waiting forever. Sad.
-             */
-err_discard:
-            for (size_t j = i + 1; j < stop; j++)
-                if (from[j] != XCB_ATOM_NONE)
-                    xcb_discard_reply(conn, cookies[j % SIZE].sequence);
-            return false;
         }
     }
 
-    return true;
+    /* Already pending? */
+    for (size_t i = 0; i < interner->num_pending; i++) {
+        if (interner->pending[i].from == atom) {
+            if (interner->num_copies == ARRAY_SIZE(interner->copies)) {
+                x11_atom_interner_round_trip(interner);
+                goto retry;
+            }
+
+            size_t idx = interner->num_copies++;
+            interner->copies[idx].from = atom;
+            interner->copies[idx].out = out;
+            return;
+        }
+    }
+
+    /* We have to send a GetAtomName request */
+    if (interner->num_pending == ARRAY_SIZE(interner->pending)) {
+        x11_atom_interner_round_trip(interner);
+        assert(interner->num_pending < ARRAY_SIZE(interner->pending));
+    }
+    size_t idx = interner->num_pending++;
+    interner->pending[idx].from = atom;
+    interner->pending[idx].out = out;
+    interner->pending[idx].cookie = xcb_get_atom_name(interner->conn, atom);
 }
 
-bool
+void
-adopt_atom(struct xkb_context *ctx, xcb_connection_t *conn, xcb_atom_t atom,
+x11_atom_interner_adopt_atoms(struct x11_atom_interner *interner,
-           xkb_atom_t *out)
+                              const xcb_atom_t *from, xkb_atom_t *to,
+                              size_t count)
 {
-    return adopt_atoms(ctx, conn, &atom, out, 1);
+    for (size_t i = 0; i < count; i++) {
+        x11_atom_interner_adopt_atom(interner, from[i], &to[i]);
+    }
+}
+
+void x11_atom_interner_round_trip(struct x11_atom_interner *interner) {
+    struct xkb_context *ctx = interner->ctx;
+    xcb_connection_t *conn = interner->conn;
+
+    /* Can be NULL in case the malloc failed. */
+    struct x11_atom_cache *cache = get_cache(ctx, conn);
+
+    for (size_t i = 0; i < interner->num_pending; i++) {
+        xcb_get_atom_name_reply_t *reply;
+
+        reply = xcb_get_atom_name_reply(conn, interner->pending[i].cookie, NULL);
+        if (!reply) {
+            interner->had_error = true;
+            continue;
+        }
+        xcb_atom_t x11_atom = interner->pending[i].from;
+        xkb_atom_t atom = xkb_atom_intern(ctx,
+                                          xcb_get_atom_name_name(reply),
+                                          xcb_get_atom_name_name_length(reply));
+        free(reply);
+
+        if (cache && cache->len < ARRAY_SIZE(cache->cache)) {
+            size_t idx = cache->len++;
+            cache->cache[idx].from = x11_atom;
+            cache->cache[idx].to = atom;
+        }
+
+        *interner->pending[i].out = atom;
+
+        for (size_t j = 0; j < interner->num_copies; j++) {
+            if (interner->copies[j].from == x11_atom)
+                *interner->copies[j].out = atom;
+        }
+    }
+
+    interner->num_pending = 0;
+    interner->num_copies = 0;
 }

src/x11/x11-priv.h

@@ -33,22 +33,44 @@
 bool
 get_atom_name(xcb_connection_t *conn, xcb_atom_t atom, char **out);
 
-/*
+struct x11_atom_interner {
- * Make a xkb_atom_t's from X atoms (prefer to send as many as possible
+    struct xkb_context *ctx;
- * at once, to avoid many roundtrips).
+    xcb_connection_t *conn;
- *
+    bool had_error;
- * TODO: We can make this more flexible, such that @to doesn't have to
+    /* Atoms for which we send a GetAtomName request */
- *       be sequential. Then we can convert most adopt_atom() calls to
+    struct {
- *       adopt_atoms().
+        xcb_atom_t from;
- *       Atom caching would also likely be useful for avoiding quite a
+        xkb_atom_t *out;
- *       few requests.
+        xcb_get_atom_name_cookie_t cookie;
- */
+    } pending[128];
-bool
+    size_t num_pending;
-adopt_atoms(struct xkb_context *ctx, xcb_connection_t *conn,
+    /* Atoms which were already pending but queried again */
-            const xcb_atom_t *from, xkb_atom_t *to, size_t count);
+    struct {
+        xcb_atom_t from;
+        xkb_atom_t *out;
+    } copies[128];
+    size_t num_copies;
+};
 
-bool
+void
-adopt_atom(struct xkb_context *ctx, xcb_connection_t *conn, xcb_atom_t atom,
+x11_atom_interner_init(struct x11_atom_interner *interner,
-           xkb_atom_t *out);
+                       struct xkb_context *ctx, xcb_connection_t *conn);
+
+void
+x11_atom_interner_round_trip(struct x11_atom_interner *interner);
+
+/*
+ * Make a xkb_atom_t's from X atoms. The actual write is delayed until the next
+ * call to x11_atom_interner_round_trip() or when too many atoms are pending.
+ */
+void
+x11_atom_interner_adopt_atom(struct x11_atom_interner *interner,
+                             const xcb_atom_t atom, xkb_atom_t *out);
+
+
+void
+x11_atom_interner_adopt_atoms(struct x11_atom_interner *interner,
+                              const xcb_atom_t *from, xkb_atom_t *to,
+                              size_t count);
 
 #endif