Commit 40c00b472144d1684d2fb97cafef39ef59f21b28

Uli Schlachter 2021-03-07T07:42:28

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>

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
diff --git a/src/x11/keymap.c b/src/x11/keymap.c
index f5b368f..38be217 100644
--- a/src/x11/keymap.c
+++ b/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))
-            goto fail;
-
-        if (!adopt_atoms(keymap->ctx, conn,
-                         kt_level_names_iter, type->level_names,
-                         wire_num_levels))
-            goto fail;
+        x11_atom_interner_adopt_atom(interner, wire_type_name, &type->name);
+        x11_atom_interner_adopt_atoms(interner, kt_level_names_iter,
+                                     type->level_names, wire_num_levels);
 
         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))
-                return false;
+            x11_atom_interner_adopt_atom(interner, wire, &led->name);
 
             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))
-                return false;
+            x11_atom_interner_adopt_atom(interner, wire, &mod->name);
 
             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,
-                     iter, keymap->group_names, length))
-        goto fail;
+    x11_atom_interner_adopt_atoms(interner, iter, keymap->group_names, length);
 
     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_indicator_names(keymap, conn, reply, &list) ||
-        !get_vmod_names(keymap, conn, reply, &list) ||
-        !get_group_names(keymap, conn, reply, &list) ||
+        !get_type_names(keymap, interner, reply, &list) ||
+        !get_indicator_names(keymap, interner, reply, &list) ||
+        !get_vmod_names(keymap, interner, 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;
 }
diff --git a/src/x11/util.c b/src/x11/util.c
index 660d885..766e9a0 100644
--- a/src/x11/util.c
+++ b/src/x11/util.c
@@ -169,14 +169,9 @@ struct x11_atom_cache {
     size_t len;
 };
 
-bool
-adopt_atoms(struct xkb_context *ctx, xcb_connection_t *conn,
-            const xcb_atom_t *from, xkb_atom_t *to, const size_t count)
+static struct x11_atom_cache *
+get_cache(struct xkb_context *ctx, xcb_connection_t *conn)
 {
-    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));
-
-    /* Send and collect the atoms in batches of reasonable SIZE. */
-    for (size_t batch = 0; batch < num_batches; batch++) {
-        const size_t start = batch * SIZE;
-        const size_t stop = MIN((batch + 1) * SIZE, count);
-
-        /* Send. */
-        for (size_t i = start; i < stop; i++) {
-            bool cache_hit = false;
-            if (cache) {
-                for (size_t c = 0; c < cache->len; c++) {
-                    if (cache->cache[c].from == from[i]) {
-                        to[i] = cache->cache[c].to;
-                        cache_hit = true;
-                        break;
-                    }
-                }
+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;
+}
+
+void
+x11_atom_interner_adopt_atom(struct x11_atom_interner *interner,
+                             const xcb_atom_t atom, xkb_atom_t *out)
+{
+    *out = 0;
+
+    /* Can be NULL in case the malloc failed. */
+    struct x11_atom_cache *cache = get_cache(interner->ctx, interner->conn);
+
+retry:
+
+    /* Already in the cache? */
+    if (cache) {
+        for (size_t c = 0; c < cache->len; c++) {
+            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;
+    /* 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;
+            }
 
-            if (from[i] == XCB_ATOM_NONE)
-                continue;
+            size_t idx = interner->num_copies++;
+            interner->copies[idx].from = atom;
+            interner->copies[idx].out = out;
+            return;
+        }
+    }
 
-            /* Was filled from cache. */
-            if (to[i] != 0)
-                continue;
+    /* 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);
+}
 
-            reply = xcb_get_atom_name_reply(conn, cookies[i % SIZE], NULL);
-            if (!reply)
-                goto err_discard;
+void
+x11_atom_interner_adopt_atoms(struct x11_atom_interner *interner,
+                              const xcb_atom_t *from, xkb_atom_t *to,
+                              size_t count)
+{
+    for (size_t i = 0; i < count; i++) {
+        x11_atom_interner_adopt_atom(interner, from[i], &to[i]);
+    }
+}
 
-            to[i] = xkb_atom_intern(ctx,
-                                    xcb_get_atom_name_name(reply),
-                                    xcb_get_atom_name_name_length(reply));
-            free(reply);
+void x11_atom_interner_round_trip(struct x11_atom_interner *interner) {
+    struct xkb_context *ctx = interner->ctx;
+    xcb_connection_t *conn = interner->conn;
 
-            if (to[i] == XKB_ATOM_NONE)
-                goto err_discard;
+    /* Can be NULL in case the malloc failed. */
+    struct x11_atom_cache *cache = get_cache(ctx, conn);
 
-            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];
-            }
+    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 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;
+        if (cache && cache->len < ARRAY_SIZE(cache->cache)) {
+            size_t idx = cache->len++;
+            cache->cache[idx].from = x11_atom;
+            cache->cache[idx].to = atom;
         }
-    }
 
-    return true;
-}
+        *interner->pending[i].out = atom;
 
-bool
-adopt_atom(struct xkb_context *ctx, xcb_connection_t *conn, xcb_atom_t atom,
-           xkb_atom_t *out)
-{
-    return adopt_atoms(ctx, conn, &atom, out, 1);
+        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;
 }
diff --git a/src/x11/x11-priv.h b/src/x11/x11-priv.h
index 3a19e99..5b7f5c2 100644
--- a/src/x11/x11-priv.h
+++ b/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 {
+    struct xkb_context *ctx;
+    xcb_connection_t *conn;
+    bool had_error;
+    /* Atoms for which we send a GetAtomName request */
+    struct {
+        xcb_atom_t from;
+        xkb_atom_t *out;
+        xcb_get_atom_name_cookie_t cookie;
+    } pending[128];
+    size_t num_pending;
+    /* Atoms which were already pending but queried again */
+    struct {
+        xcb_atom_t from;
+        xkb_atom_t *out;
+    } copies[128];
+    size_t num_copies;
+};
+
+void
+x11_atom_interner_init(struct x11_atom_interner *interner,
+                       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 (prefer to send as many as possible
- * at once, to avoid many roundtrips).
- *
- * TODO: We can make this more flexible, such that @to doesn't have to
- *       be sequential. Then we can convert most adopt_atom() calls to
- *       adopt_atoms().
- *       Atom caching would also likely be useful for avoiding quite a
- *       few requests.
+ * 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.
  */
-bool
-adopt_atoms(struct xkb_context *ctx, xcb_connection_t *conn,
-            const xcb_atom_t *from, xkb_atom_t *to, size_t count);
+void
+x11_atom_interner_adopt_atom(struct x11_atom_interner *interner,
+                             const xcb_atom_t atom, xkb_atom_t *out);
 
-bool
-adopt_atom(struct xkb_context *ctx, xcb_connection_t *conn, 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