Minor improvements to the shaped run cache

- Build the cache key once per run rather than once in shaped_run_get()
  and again in shaped_run_put(), which had to walk the cells and query
  the text cache a second time on every cache miss

- Do not zero the entire groups array on a cache hit, the groups that are
  actually read are all overwritten by the cached ones

- Drop the cache when an arena allocation or the insert fails, instead of
  leaking the space allocated for the entry, since monotonic arenas
  cannot free individual allocations

kitty/fonts.c

@@ -1475,8 +1475,9 @@ ensure_group_capacity(size_t num_cells, size_t num_groups) {
 static bool
 restore_shaped_run(CPUCell *first_cpu_cell, GPUCell *first_gpu_cell, index_type num_cells, const ShapedRun *cached) {
     if (!ensure_group_capacity(num_cells, cached->num_groups)) return false;
-    zero_at_ptr_count(group_state.groups, group_state.groups_capacity);
+    // every group up to group_idx is overwritten below and nothing reads past it, so zeroing the whole array is unnecessary
     if (cached->num_groups) memcpy(group_state.groups, cached->groups, cached->num_groups * sizeof(Group));
+    else zero_at_ptr(group_state.groups);
     if (cached->num_glyphs) {
         if (!ensure_group_glyph_storage(cached->num_glyphs)) return false;
         memcpy(group_state.info_storage, cached->info, cached->num_glyphs * sizeof(cached->info[0]));
@@ -1899,20 +1900,7 @@ shape_run(
         hb_buffer_serialize_glyphs(harfbuzz_buffer, 0, group_state.num_glyphs, dbuf, sizeof(dbuf), NULL, harfbuzz_font_for_face(font->face), HB_BUFFER_SERIALIZE_FORMAT_TEXT, HB_BUFFER_SERIALIZE_FLAG_DEFAULT | HB_BUFFER_SERIALIZE_FLAG_GLYPH_EXTENTS);
         printf("\n%s\n", dbuf);
 #endif
-    shaped_run_put(
-        font->shaped_run_hash_table,
-        first_cpu_cell,
-        num_cells,
-        tc,
-        disable_ligature,
-        OPT(force_ltr),
-        key_scale,
-        subscale,
-        G(info),
-        G(positions),
-        (unsigned)G(num_glyphs),
-        G(groups),
-        (unsigned)(G(group_idx) + 1));
+    shaped_run_put(font->shaped_run_hash_table, G(info), G(positions), (unsigned)G(num_glyphs), G(groups), (unsigned)(G(group_idx) + 1));
     if (scale != 1.f) {
         apply_scale_to_font_group(fg, NULL);
         if (!face_apply_scaling(font->face, (FONTS_DATA_HANDLE)fg) && PyErr_Occurred()) PyErr_Print();

kitty/shaped-run-cache.c

@@ -7,6 +7,9 @@
 
 #include "shaped-run-cache.h"
 
+// Note that these limits apply to every Font, of which there are at least a
+// dozen per FontGroup once fallback and symbol fonts are in use, so the total
+// memory used by shaped run caches is a multiple of SHAPED_RUN_MAX_BYTES.
 #define SHAPED_RUN_MAX_CELLS 32u
 #define SHAPED_RUN_MAX_ENTRIES 2048u
 #define SHAPED_RUN_MAX_BYTES (1024u * 1024u)
@@ -78,6 +81,7 @@ typedef struct HashTable {
     struct {
         ShapedRunKey *key;
         size_t capacity;
+        bool key_is_valid; // true when key describes the run from the last shaped_run_get()
     } scratch;
 } HashTable;
 
@@ -161,11 +165,14 @@ shaped_run_get(
     ShapedRun *result) {
     HashTable *ht = (HashTable *)map;
     if (!ht) return false;
+    ht->scratch.key_is_valid = false;
     if (shaped_run_too_long(num_cells)) return false;
     if (!fill_key(ht, cells, num_cells, tc, disable_ligature, force_ltr, scale, subscale)) return false;
-#define scratch ht->scratch.key
-    shaped_run_map_itr n = vt_get(&ht->table, scratch);
-    if (vt_is_end(n)) return false;
+    shaped_run_map_itr n = vt_get(&ht->table, ht->scratch.key);
+    if (vt_is_end(n)) {
+        ht->scratch.key_is_valid = true; // so that shaped_run_put() can re-use it
+        return false;
+    }
     ShapedRunValue *v = n.data->val;
     result->num_glyphs = v->num_glyphs;
     result->num_groups = v->num_groups;
@@ -173,7 +180,6 @@ shaped_run_get(
     result->positions = v->num_glyphs ? val_positions(v) : NULL;
     result->groups = v->num_groups ? val_groups(v) : NULL;
     return true;
-#undef scratch
 }
 
 static size_t
@@ -215,31 +221,28 @@ shaped_run_drop(HashTable *ht) {
 void
 shaped_run_put(
     SHAPED_RUN_MAP_HANDLE map,
-    const CPUCell *cells,
-    index_type num_cells,
-    const TextCache *tc,
-    bool disable_ligature,
-    bool force_ltr,
-    uint8_t scale,
-    uint8_t subscale,
     const hb_glyph_info_t *info,
     const hb_glyph_position_t *positions,
     unsigned num_glyphs,
     const ShapeGroup *groups,
     unsigned num_groups) {
     HashTable *ht = (HashTable *)map;
-    if (!ht) return;
-    if (shaped_run_too_long(num_cells)) return;
+    // the key for this run was built by the shaped_run_get() call that missed,
+    // without it we have no way of knowing what run these glyphs belong to
+    if (!ht || !ht->scratch.key_is_valid) return;
+    ht->scratch.key_is_valid = false;
     if ((num_glyphs && (!info || !positions)) || (num_groups && !groups)) return;
-    if (!fill_key(ht, cells, num_cells, tc, disable_ligature, force_ltr, scale, subscale)) return;
 #define scratch ht->scratch.key
-    if (!vt_is_end(vt_get(&ht->table, scratch))) return;
     if (shaped_run_at_cap(ht)) shaped_run_drop(ht);
     ShapedRunKey *key = Key_get(&ht->keys, sizeof(ShapedRunKey) + scratch->keysz_in_bytes);
-    if (!key) return;
+    ShapedRunValue *val = key ? Val_get(&ht->vals, val_size(num_glyphs, num_groups)) : NULL;
+    // the arenas cannot free individual allocations, so on failure drop the
+    // whole cache rather than leaking what was allocated for this entry
+    if (!val) {
+        shaped_run_drop(ht);
+        return;
+    }
     memcpy(key, scratch, sizeof(scratch[0]) + scratch->keysz_in_bytes);
-    ShapedRunValue *val = Val_get(&ht->vals, val_size(num_glyphs, num_groups));
-    if (!val) return;
     val->num_glyphs = num_glyphs;
     val->num_groups = num_groups;
     if (num_glyphs) {
@@ -247,6 +250,6 @@ shaped_run_put(
         memcpy(val_positions(val), positions, num_glyphs * sizeof(positions[0]));
     }
     if (num_groups) memcpy(val_groups(val), groups, num_groups * sizeof(groups[0]));
-    if (vt_is_end(vt_insert(&ht->table, key, val))) return;
+    if (vt_is_end(vt_insert(&ht->table, key, val))) shaped_run_drop(ht);
 #undef scratch
 }

kitty/shaped-run-cache.h

@@ -41,15 +41,13 @@ bool shaped_run_get(
     uint8_t scale,
     uint8_t subscale,
     ShapedRun *result);
+// Store the results of shaping the run that the immediately preceding
+// shaped_run_get() call looked up and did not find. Building the key is not
+// cheap, so it is re-used rather than computed a second time. Does nothing if
+// there was no such call, i.e. if the run was found in the cache or is not
+// cacheable at all.
 void shaped_run_put(
     SHAPED_RUN_MAP_HANDLE map,
-    const CPUCell *cells,
-    index_type num_cells,
-    const TextCache *tc,
-    bool disable_ligature,
-    bool force_ltr,
-    uint8_t scale,
-    uint8_t subscale,
     const hb_glyph_info_t *info,
     const hb_glyph_position_t *positions,
     unsigned num_glyphs,