From 6315faed20ce31a60015ba051239d386ea355158 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Thu, 15 Feb 2024 21:08:48 +0000 Subject: [PATCH 1/3] cache: redesign cache uses an invasive design now, a la list.h and uthash. This get rid of the ugly integer to pointer cache, and gives us freedom of what we can put into the cache. Signed-off-by: Yuxuan Shui --- src/atom.c | 38 +++++++++++++++----- src/atom.h | 26 ++++++++++---- src/cache.c | 100 ++++++++++++++++------------------------------------ src/cache.h | 48 ++++++++++++++++--------- src/list.h | 13 +------ src/utils.h | 14 ++++++++ 6 files changed, 126 insertions(+), 113 deletions(-) diff --git a/src/atom.c b/src/atom.c index bf94232..76fc57f 100644 --- a/src/atom.c +++ b/src/atom.c @@ -2,14 +2,19 @@ #include #include "atom.h" +#include "cache.h" #include "common.h" -#include "utils.h" +#include "compiler.h" #include "log.h" +#include "utils.h" -static inline void *atom_getter(void *ud, const char *atom_name, int *err) { - xcb_connection_t *c = ud; +static inline int +atom_getter(struct cache *cache, const char *atom_name, struct cache_handle **value) { + struct atom *atoms = container_of(cache, struct atom, c); xcb_intern_atom_reply_t *reply = xcb_intern_atom_reply( - c, xcb_intern_atom(c, 0, to_u16_checked(strlen(atom_name)), atom_name), NULL); + atoms->conn, + xcb_intern_atom(atoms->conn, 0, to_u16_checked(strlen(atom_name)), atom_name), + NULL); xcb_atom_t atom = XCB_NONE; if (reply) { @@ -18,9 +23,19 @@ static inline void *atom_getter(void *ud, const char *atom_name, int *err) { free(reply); } else { log_error("Failed to intern atoms"); - *err = 1; + return -1; } - return (void *)(intptr_t)atom; + + struct atom_entry *entry = ccalloc(1, struct atom_entry); + entry->atom = atom; + *value = &entry->entry; + return 0; +} + +static inline void +atom_entry_free(struct cache *cache attr_unused, struct cache_handle *handle) { + struct atom_entry *entry = cache_entry(handle, struct atom_entry, entry); + free(entry); } /** @@ -28,11 +43,16 @@ static inline void *atom_getter(void *ud, const char *atom_name, int *err) { */ struct atom *init_atoms(xcb_connection_t *c) { auto atoms = ccalloc(1, struct atom); - atoms->c = new_cache((void *)c, atom_getter, NULL); -#define ATOM_GET(x) \ - atoms->a##x = (xcb_atom_t)(intptr_t)cache_get_or_fetch(atoms->c, #x, NULL) + atoms->conn = c; + cache_init(&atoms->c, atom_getter); +#define ATOM_GET(x) atoms->a##x = get_atom(atoms, #x) LIST_APPLY(ATOM_GET, SEP_COLON, ATOM_LIST1); LIST_APPLY(ATOM_GET, SEP_COLON, ATOM_LIST2); #undef ATOM_GET return atoms; } + +void destroy_atoms(struct atom *a) { + cache_invalidate_all(&a->c, atom_entry_free); + free(a); +} diff --git a/src/atom.h b/src/atom.h index 91c2829..6787331 100644 --- a/src/atom.h +++ b/src/atom.h @@ -4,6 +4,7 @@ #include #include "cache.h" +#include "log.h" #include "meta.h" // clang-format off @@ -54,22 +55,33 @@ #define ATOM_DEF(x) xcb_atom_t a##x struct atom { - struct cache *c; + xcb_connection_t *conn; + struct cache c; LIST_APPLY(ATOM_DEF, SEP_COLON, ATOM_LIST1); LIST_APPLY(ATOM_DEF, SEP_COLON, ATOM_LIST2); }; +struct atom_entry { + struct cache_handle entry; + xcb_atom_t atom; +}; + +/// Create a new atom object with a xcb connection, note that this atom object will hold a +/// reference to the connection, so the caller must keep the connection alive until the +/// atom object is destroyed. struct atom *init_atoms(xcb_connection_t *); static inline xcb_atom_t get_atom(struct atom *a, const char *key) { - return (xcb_atom_t)(intptr_t)cache_get_or_fetch(a->c, key, NULL); + struct cache_handle *entry = NULL; + if (cache_get_or_fetch(&a->c, key, &entry) < 0) { + log_error("Failed to get atom %s", key); + return XCB_NONE; + } + return cache_entry(entry, struct atom_entry, entry)->atom; } static inline xcb_atom_t get_atom_cached(struct atom *a, const char *key) { - return (xcb_atom_t)(intptr_t)cache_get(a->c, key); + return cache_entry(cache_get(&a->c, key), struct atom_entry, entry)->atom; } -static inline void destroy_atoms(struct atom *a) { - cache_free(a->c); - free(a); -} +void destroy_atoms(struct atom *a); diff --git a/src/cache.c b/src/cache.c index 8c5d91d..d25a22f 100644 --- a/src/cache.c +++ b/src/cache.c @@ -1,94 +1,56 @@ #include -#include "compiler.h" -#include "utils.h" #include "cache.h" -struct cache_entry { - char *key; - void *value; - UT_hash_handle hh; -}; - -struct cache { - cache_getter_t getter; - cache_free_t free; - void *user_data; - struct cache_entry *entries; -}; - -static inline struct cache_entry *cache_get_entry(struct cache *c, const char *key) { - struct cache_entry *e; +struct cache_handle *cache_get(struct cache *c, const char *key) { + struct cache_handle *e; HASH_FIND_STR(c->entries, key, e); return e; } -void *cache_get(struct cache *c, const char *key) { - struct cache_entry *e = cache_get_entry(c, key); - return e ? e->value : NULL; +int cache_get_or_fetch(struct cache *c, const char *key, struct cache_handle **value) { + *value = cache_get(c, key); + if (*value) { + return 0; + } + + int err = c->getter(c, key, value); + assert(err <= 0); + if (err < 0) { + return err; + } + (*value)->key = strdup(key); + + HASH_ADD_STR(c->entries, key, *value); + return 1; } -void *cache_get_or_fetch(struct cache *c, const char *key, int *err) { - struct cache_entry *e = cache_get_entry(c, key); - if (e) { - return e->value; - } - - int tmperr; - if (!err) { - err = &tmperr; - } - - *err = 0; - e = ccalloc(1, struct cache_entry); - e->key = strdup(key); - e->value = c->getter(c->user_data, key, err); - if (*err) { - free(e->key); - free(e); - return NULL; - } - - HASH_ADD_STR(c->entries, key, e); - return e->value; -} - -static inline void cache_invalidate_impl(struct cache *c, struct cache_entry *e) { - if (c->free) { - c->free(c->user_data, e->value); - } +static inline void +cache_invalidate_impl(struct cache *c, struct cache_handle *e, cache_free_t free_fn) { free(e->key); HASH_DEL(c->entries, e); - free(e); + if (free_fn) { + free_fn(c, e); + } } -void cache_invalidate(struct cache *c, const char *key) { - struct cache_entry *e; +void cache_invalidate(struct cache *c, const char *key, cache_free_t free_fn) { + struct cache_handle *e; HASH_FIND_STR(c->entries, key, e); if (e) { - cache_invalidate_impl(c, e); + cache_invalidate_impl(c, e, free_fn); } } -void cache_invalidate_all(struct cache *c) { - struct cache_entry *e, *tmpe; +void cache_invalidate_all(struct cache *c, cache_free_t free_fn) { + struct cache_handle *e, *tmpe; HASH_ITER(hh, c->entries, e, tmpe) { - cache_invalidate_impl(c, e); + cache_invalidate_impl(c, e, free_fn); } } -void *cache_free(struct cache *c) { - void *ret = c->user_data; - cache_invalidate_all(c); - free(c); - return ret; -} - -struct cache *new_cache(void *ud, cache_getter_t getter, cache_free_t f) { - auto c = ccalloc(1, struct cache); - c->user_data = ud; - c->getter = getter; - c->free = f; - return c; +void cache_init(struct cache *cache, cache_getter_t getter) { + cache->getter = getter; + cache->entries = NULL; } diff --git a/src/cache.h b/src/cache.h index 2ac1a01..6060919 100644 --- a/src/cache.h +++ b/src/cache.h @@ -1,30 +1,46 @@ #pragma once +#include +#include "utils.h" + +#define cache_entry(ptr, type, member) container_of(ptr, type, member) + struct cache; +struct cache_handle; -typedef void *(*cache_getter_t)(void *user_data, const char *key, int *err); -typedef void (*cache_free_t)(void *user_data, void *data); +/// User-provided function to fetch a value for the cache, when it's not present. +/// Should return 0 if the value is fetched successfully, and a negative number if the +/// value cannot be fetched. Getter doesn't need to initialize fields of `struct +/// cache_handle`. +typedef int (*cache_getter_t)(struct cache *, const char *key, struct cache_handle **value); +typedef void (*cache_free_t)(struct cache *, struct cache_handle *value); -/// Create a cache with `getter`, and a free function `f` which is used to free the cache -/// value when they are invalidated. -/// -/// `user_data` will be passed to `getter` and `f` when they are called. -struct cache *new_cache(void *user_data, cache_getter_t getter, cache_free_t f); +struct cache { + cache_getter_t getter; + struct cache_handle *entries; +}; + +struct cache_handle { + char *key; + UT_hash_handle hh; +}; + +/// Initialize a cache with `getter` +void cache_init(struct cache *cache, cache_getter_t getter); /// Get a value from the cache. If the value doesn't present in the cache yet, the /// getter will be called, and the returned value will be stored into the cache. -void *cache_get_or_fetch(struct cache *, const char *key, int *err); +/// Returns 0 if the value is already present in the cache, 1 if the value is fetched +/// successfully, and a negative number if the value cannot be fetched. +int cache_get_or_fetch(struct cache *, const char *key, struct cache_handle **value); /// Get a value from the cache. If the value doesn't present in the cache, NULL will be /// returned. -void *cache_get(struct cache *, const char *key); +struct cache_handle *cache_get(struct cache *, const char *key); /// Invalidate a value in the cache. -void cache_invalidate(struct cache *, const char *key); +void cache_invalidate(struct cache *, const char *key, cache_free_t free_fn); -/// Invalidate all values in the cache. -void cache_invalidate_all(struct cache *); - -/// Invalidate all values in the cache and free it. Returns the user data passed to -/// `new_cache` -void *cache_free(struct cache *); +/// Invalidate all values in the cache. After this call, `struct cache` holds no allocated +/// memory, and can be discarded. +void cache_invalidate_all(struct cache *, cache_free_t free_fn); diff --git a/src/list.h b/src/list.h index 19e2c2c..d63a764 100644 --- a/src/list.h +++ b/src/list.h @@ -2,18 +2,7 @@ #include #include -/** - * container_of - cast a member of a structure out to the containing structure - * @ptr: the pointer to the member. - * @type: the type of the container struct this is embedded in. - * @member: the name of the member within the struct. - * - */ -#define container_of(ptr, type, member) \ - ({ \ - const __typeof__(((type *)0)->member) *__mptr = (ptr); \ - (type *)((char *)__mptr - offsetof(type, member)); \ - }) +#include "utils.h" struct list_node { struct list_node *next, *prev; diff --git a/src/utils.h b/src/utils.h index 73cb711..5a42d99 100644 --- a/src/utils.h +++ b/src/utils.h @@ -117,6 +117,20 @@ safe_isnan(double a) { ASSERT_IN_RANGE(__to_tmp, 0, max); \ (uint32_t) __to_tmp; \ }) + +/** + * container_of - cast a member of a structure out to the containing structure + * @ptr: the pointer to the member. + * @type: the type of the container struct this is embedded in. + * @member: the name of the member within the struct. + * + */ +#define container_of(ptr, type, member) \ + ({ \ + const __typeof__(((type *)0)->member) *__mptr = (ptr); \ + (type *)((char *)__mptr - offsetof(type, member)); \ + }) + /** * Normalize an int value to a specific range. * From a93bbc30e5d22a497e3158a810457c3b054003fd Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Fri, 16 Feb 2024 00:31:53 +0000 Subject: [PATCH 2/3] atom: get_atom now requires explicit xcb_connection_t This is to make the access to X server more explicit, and make managing the lifetime of xcb_connection_t a bit easier. Signed-off-by: Yuxuan Shui --- src/atom.c | 24 +++++++++++++++--------- src/atom.h | 18 ++++-------------- src/c2.c | 2 +- src/cache.c | 10 +++------- src/cache.h | 12 ++++++------ src/picom.c | 2 +- 6 files changed, 30 insertions(+), 38 deletions(-) diff --git a/src/atom.c b/src/atom.c index 76fc57f..8ef699e 100644 --- a/src/atom.c +++ b/src/atom.c @@ -8,13 +8,11 @@ #include "log.h" #include "utils.h" -static inline int -atom_getter(struct cache *cache, const char *atom_name, struct cache_handle **value) { - struct atom *atoms = container_of(cache, struct atom, c); +static inline int atom_getter(struct cache *cache attr_unused, const char *atom_name, + struct cache_handle **value, void *user_data) { + xcb_connection_t *c = user_data; xcb_intern_atom_reply_t *reply = xcb_intern_atom_reply( - atoms->conn, - xcb_intern_atom(atoms->conn, 0, to_u16_checked(strlen(atom_name)), atom_name), - NULL); + c, xcb_intern_atom(c, 0, to_u16_checked(strlen(atom_name)), atom_name), NULL); xcb_atom_t atom = XCB_NONE; if (reply) { @@ -38,14 +36,22 @@ atom_entry_free(struct cache *cache attr_unused, struct cache_handle *handle) { free(entry); } +xcb_atom_t get_atom(struct atom *a, const char *key, xcb_connection_t *c) { + struct cache_handle *entry = NULL; + if (cache_get_or_fetch(&a->c, key, &entry, c, atom_getter) < 0) { + log_error("Failed to get atom %s", key); + return XCB_NONE; + } + return cache_entry(entry, struct atom_entry, entry)->atom; +} + /** * Create a new atom structure and fetch all predefined atoms */ struct atom *init_atoms(xcb_connection_t *c) { auto atoms = ccalloc(1, struct atom); - atoms->conn = c; - cache_init(&atoms->c, atom_getter); -#define ATOM_GET(x) atoms->a##x = get_atom(atoms, #x) + atoms->c = CACHE_INIT; +#define ATOM_GET(x) atoms->a##x = get_atom(atoms, #x, c) LIST_APPLY(ATOM_GET, SEP_COLON, ATOM_LIST1); LIST_APPLY(ATOM_GET, SEP_COLON, ATOM_LIST2); #undef ATOM_GET diff --git a/src/atom.h b/src/atom.h index 6787331..d5a41d5 100644 --- a/src/atom.h +++ b/src/atom.h @@ -55,7 +55,6 @@ #define ATOM_DEF(x) xcb_atom_t a##x struct atom { - xcb_connection_t *conn; struct cache c; LIST_APPLY(ATOM_DEF, SEP_COLON, ATOM_LIST1); LIST_APPLY(ATOM_DEF, SEP_COLON, ATOM_LIST2); @@ -66,20 +65,11 @@ struct atom_entry { xcb_atom_t atom; }; -/// Create a new atom object with a xcb connection, note that this atom object will hold a -/// reference to the connection, so the caller must keep the connection alive until the -/// atom object is destroyed. -struct atom *init_atoms(xcb_connection_t *); - -static inline xcb_atom_t get_atom(struct atom *a, const char *key) { - struct cache_handle *entry = NULL; - if (cache_get_or_fetch(&a->c, key, &entry) < 0) { - log_error("Failed to get atom %s", key); - return XCB_NONE; - } - return cache_entry(entry, struct atom_entry, entry)->atom; -} +/// Create a new atom object with a xcb connection. `struct atom` does not hold +/// a reference to the connection. +struct atom *init_atoms(xcb_connection_t *c); +xcb_atom_t get_atom(struct atom *a, const char *key, xcb_connection_t *c); static inline xcb_atom_t get_atom_cached(struct atom *a, const char *key) { return cache_entry(cache_get(&a->c, key), struct atom_entry, entry)->atom; } diff --git a/src/c2.c b/src/c2.c index 614c456..140d274 100644 --- a/src/c2.c +++ b/src/c2.c @@ -1047,7 +1047,7 @@ static bool c2_l_postprocess(session_t *ps, c2_l_t *pleaf) { // Get target atom if it's not a predefined one if (pleaf->predef == C2_L_PUNDEFINED) { - pleaf->tgtatom = get_atom(ps->atoms, pleaf->tgt); + pleaf->tgtatom = get_atom(ps->atoms, pleaf->tgt, ps->c.c); if (!pleaf->tgtatom) { log_error("Failed to get atom for target \"%s\".", pleaf->tgt); return false; diff --git a/src/cache.c b/src/cache.c index d25a22f..e297c4f 100644 --- a/src/cache.c +++ b/src/cache.c @@ -8,13 +8,14 @@ struct cache_handle *cache_get(struct cache *c, const char *key) { return e; } -int cache_get_or_fetch(struct cache *c, const char *key, struct cache_handle **value) { +int cache_get_or_fetch(struct cache *c, const char *key, struct cache_handle **value, + void *user_data, cache_getter_t getter) { *value = cache_get(c, key); if (*value) { return 0; } - int err = c->getter(c, key, value); + int err = getter(c, key, value, user_data); assert(err <= 0); if (err < 0) { return err; @@ -49,8 +50,3 @@ void cache_invalidate_all(struct cache *c, cache_free_t free_fn) { cache_invalidate_impl(c, e, free_fn); } } - -void cache_init(struct cache *cache, cache_getter_t getter) { - cache->getter = getter; - cache->entries = NULL; -} diff --git a/src/cache.h b/src/cache.h index 6060919..a50ec4b 100644 --- a/src/cache.h +++ b/src/cache.h @@ -12,27 +12,27 @@ struct cache_handle; /// Should return 0 if the value is fetched successfully, and a negative number if the /// value cannot be fetched. Getter doesn't need to initialize fields of `struct /// cache_handle`. -typedef int (*cache_getter_t)(struct cache *, const char *key, struct cache_handle **value); +typedef int (*cache_getter_t)(struct cache *, const char *key, + struct cache_handle **value, void *user_data); typedef void (*cache_free_t)(struct cache *, struct cache_handle *value); struct cache { - cache_getter_t getter; struct cache_handle *entries; }; +static const struct cache CACHE_INIT = {NULL}; + struct cache_handle { char *key; UT_hash_handle hh; }; -/// Initialize a cache with `getter` -void cache_init(struct cache *cache, cache_getter_t getter); - /// Get a value from the cache. If the value doesn't present in the cache yet, the /// getter will be called, and the returned value will be stored into the cache. /// Returns 0 if the value is already present in the cache, 1 if the value is fetched /// successfully, and a negative number if the value cannot be fetched. -int cache_get_or_fetch(struct cache *, const char *key, struct cache_handle **value); +int cache_get_or_fetch(struct cache *, const char *key, struct cache_handle **value, + void *user_data, cache_getter_t getter); /// Get a value from the cache. If the value doesn't present in the cache, NULL will be /// returned. diff --git a/src/picom.c b/src/picom.c index dc075c1..5789351 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1346,7 +1346,7 @@ static int register_cm(session_t *ps) { log_fatal("Failed to allocate memory"); return -1; } - atom = get_atom(ps->atoms, buf); + atom = get_atom(ps->atoms, buf, ps->c.c); free(buf); xcb_get_selection_owner_reply_t *reply = xcb_get_selection_owner_reply( From 96de4f07ca1cb3c21293fbce9e25c1a5129a4159 Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Fri, 16 Feb 2024 00:42:22 +0000 Subject: [PATCH 3/3] cache: remove unused function cache_invalidate Signed-off-by: Yuxuan Shui --- src/cache.c | 9 --------- src/cache.h | 3 --- 2 files changed, 12 deletions(-) diff --git a/src/cache.c b/src/cache.c index e297c4f..e4f2927 100644 --- a/src/cache.c +++ b/src/cache.c @@ -35,15 +35,6 @@ cache_invalidate_impl(struct cache *c, struct cache_handle *e, cache_free_t free } } -void cache_invalidate(struct cache *c, const char *key, cache_free_t free_fn) { - struct cache_handle *e; - HASH_FIND_STR(c->entries, key, e); - - if (e) { - cache_invalidate_impl(c, e, free_fn); - } -} - void cache_invalidate_all(struct cache *c, cache_free_t free_fn) { struct cache_handle *e, *tmpe; HASH_ITER(hh, c->entries, e, tmpe) { diff --git a/src/cache.h b/src/cache.h index a50ec4b..8c022cf 100644 --- a/src/cache.h +++ b/src/cache.h @@ -38,9 +38,6 @@ int cache_get_or_fetch(struct cache *, const char *key, struct cache_handle **va /// returned. struct cache_handle *cache_get(struct cache *, const char *key); -/// Invalidate a value in the cache. -void cache_invalidate(struct cache *, const char *key, cache_free_t free_fn); - /// Invalidate all values in the cache. After this call, `struct cache` holds no allocated /// memory, and can be discarded. void cache_invalidate_all(struct cache *, cache_free_t free_fn);