Merge pull request #1200 from yshui/cache-redesign

src/atom.c

@@ -2,12 +2,15 @@
 #include <xcb/xcb.h>
 
 #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 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(
 	    c, xcb_intern_atom(c, 0, to_u16_checked(strlen(atom_name)), atom_name), NULL);
 
@@ -18,9 +21,28 @@ 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);
+}
+
+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;
 }
 
 /**
@@ -28,11 +50,15 @@ 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->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
 	return atoms;
 }
+
+void destroy_atoms(struct atom *a) {
+	cache_invalidate_all(&a->c, atom_entry_free);
+	free(a);
+}

src/atom.h

@@ -4,6 +4,7 @@
 #include <xcb/xcb.h>
 
 #include "cache.h"
+#include "log.h"
 #include "meta.h"
 
 // clang-format off
@@ -54,22 +55,23 @@
 #define ATOM_DEF(x) xcb_atom_t a##x
 
 struct atom {
-	struct cache *c;
+	struct cache c;
 	LIST_APPLY(ATOM_DEF, SEP_COLON, ATOM_LIST1);
 	LIST_APPLY(ATOM_DEF, SEP_COLON, ATOM_LIST2);
 };
 
-struct atom *init_atoms(xcb_connection_t *);
+struct atom_entry {
+	struct cache_handle entry;
+	xcb_atom_t atom;
+};
 
-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);
-}
+/// 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 (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);

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;

src/cache.c

@@ -1,94 +1,43 @@
 #include <uthash.h>
 
-#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,
+                       void *user_data, cache_getter_t getter) {
+	*value = cache_get(c, key);
+	if (*value) {
+		return 0;
+	}
+
+	int err = getter(c, key, value, user_data);
+	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);
-}
-
-void cache_invalidate(struct cache *c, const char *key) {
-	struct cache_entry *e;
-	HASH_FIND_STR(c->entries, key, e);
-
-	if (e) {
-		cache_invalidate_impl(c, e);
+	if (free_fn) {
+		free_fn(c, e);
 	}
 }
 
-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;
-}

src/cache.h

@@ -1,30 +1,43 @@
 #pragma once
 
+#include <uthash.h>
+#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, void *user_data);
+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 {
+	struct cache_handle *entries;
+};
+
+static const struct cache CACHE_INIT = {NULL};
+
+struct cache_handle {
+	char *key;
+	UT_hash_handle hh;
+};
 
 /// 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,
+                       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.
-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);
-
-/// 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);

src/list.h

@@ -2,18 +2,7 @@
 #include <stdbool.h>
 #include <stddef.h>
 
-/**
- * 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;

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(

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.
  *