Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions src/fmnoise/coldbrew.clj
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,11 @@
(.build cache-builder (cache-loader cache-fn))
(.build cache-builder))))

(defn cache-keys
"Returns a list of cache keys"
[^Cache cache]
(.keySet (.asMap cache)))

(defn put
"Insert/update a cache entry."
[^Cache cache key val]
Expand All @@ -102,6 +107,11 @@
[^Cache cache key]
(.invalidate cache key))

(defn invalidate-keys
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking if invalidate-all could be better name to mimic native call, but feels like all is a bit misleading because it's not "all" but listed ones 🤔

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that is why I named it this way - invalidate-all looks too bad for my liking😅🙈 Of course, we can have, for example, invalidate-all with 2 arities - cache + cache keys. Then it will be a bit more logical, but still...

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm now thinking maybe we should allow passing caching function as an argument 🤔

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean we're passing cache here, but what if we'll pass function, and this functions will extract cache using the implementation details

"Invalidate a cache entries."
[^Cache cache keys]
(.invalidateAll cache keys))

(defn lookup
"Performs cache lookup. Accepts optional function which uses cache key to calculate missing value"
([^Cache cache key]
Expand Down Expand Up @@ -139,12 +149,16 @@
(let [options (meta f)
condition-fn (-> f meta :when)
cache (build-cache options (when-not condition-fn f))]
^{::cache cache}
Copy link
Copy Markdown
Owner

@fmnoise fmnoise Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it's pretty much an implementation detail, so maybe we could add a helper for getting cache from the function using the meta? 🤔

Copy link
Copy Markdown
Author

@Aeonax Aeonax Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit tricky, but I did it=) Question is how to name it... too many caches in naming🤔 get-defcache-cache or defcache-cache or defcache-cache-object or ......

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, there is a cached function too, so I need to add a helper for both defcache and cached🤔

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now I feel like maybe storing cache in function meta/var meta is a bit messy way 🤔
maybe we could have caches registry in atom or volatile, which could allow us to have more tooling for monitoring caches size 🤔
and we could probably use defcached fully qualified function name for the cache key in the registry 🤔
but I'm not sure how should we put the cache into registry when using cached 🤔
maybe we should just add something like cache-id for manipulating the cache, and that would allow us to get rid of cached->cache or anything like that, allowing just to pass cache-id when creating cache and then using it with cache-keys and invalidate-keys 🤔

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I prefer meta because:

  • It exists on the same level of execution - just a part of the result
  • There's no extra complexity with cache IDs, storage layers, etc.
  • I was able to link it to the source caching object with minimal changes, and having access to that caching object is all I need right now.

Yes, integrating this meta into defcached and accessing it can be a bit tricky, but it works. And for the end user, it’s just a cached->cache or defcached->cached.


That said, if you have a clear idea for a different approach, I’d be really interested to see how it turns out =)

Copy link
Copy Markdown
Owner

@fmnoise fmnoise May 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meta is convenient, but it's way more complicated than it seems, as it could be attached to the object, but only of certain types, and passed along with it, and also to refs (vars, namespaces etc) which has completely different semantics and can contain objects of any type including nil 🤷🏽‍♂️
so, more straightforward way could be giving cache meaningful id, if you'll ever need to work with it and some clear way to access cache by id

(fn [& args]
(let [key (or args [])]
(if condition-fn
(cond-lookup cache key condition-fn f args)
(lookup cache key))))))

(defn cached-cache [cached-fn]
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like oily-oil 🙈
I'd try something like cached->cache or fn->cache

(-> (meta cached-fn) ::cache))

(defmacro defcached
"Creates a function which uses Caffeine Loading Cache under the hood.
Function declaration is similar to defn:
Expand Down Expand Up @@ -181,11 +195,14 @@
:else 3))
fdefn (filter some? (list name docstring args prepost))]
`(do
(def ^:private ~fname
(cached
(with-meta
(fn [~@cache-key]
~@body)
~cache-options)))
(defn ~@fdefn (~fname ~@cache-key))
(vary-meta ~name merge ~(meta name)))))
(def ^:private ~fname (cached
(with-meta
(fn [~@cache-key]
~@body)
~cache-options)))
(defn ~@fdefn (~fname ~@cache-key))
Copy link
Copy Markdown
Owner

@fmnoise fmnoise May 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe should apply cache metadata taken from (meta ~fname) to this function object rather than applying it to var?

something like

 (def ~name
         (with-meta
           (fn ~@(when docstring [docstring])
             ~@(when prepost [prepost])
             [~@args]
             (~fname ~@cache-key))
           (meta ~fname)))))

my idea here is to always put cache meta on function object, so when we need to get cache, we can use only function instead of 2 approaches: function and var

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks interesting!🤔 I didn't want to fight with all those variables, but it doesn't look as bad as I imagined.

I'm busy with other stuff this week, but on the next week I should be able to return into this pr.

(alter-meta! (var ~name) merge (meta ~fname) ~(meta name)))))
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, does alter-meta! fit better than vary-meta here?

Copy link
Copy Markdown
Author

@Aeonax Aeonax Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in PM, looks like vary-meta returns a new object with altered meta - it doesn't mutate the initial def, so it did nothing. Meta passed to defcache persisted without vary-meta help.
Unless I missed some corner case, or something else... Because this angle of interaction with meta is a bit new to me.

Copy link
Copy Markdown
Owner

@fmnoise fmnoise Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, vary-meta returns new object, same as with-meta, while alter-meta! does meta mutation, but it works on var reference level rather than the object level, so I'm not sure it will work good with 2 different approaches - object meta association on caching function using with-meta and then on var reference using alter-meta!

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also I can't remember which kind of meta did we pass on that function and for the sake of what 😅
probably just normal function meta non-related to caching at all

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember whether I tested all possibilities, but as I remember, in this case, I could have used (alter-meta! (var ~name) merge (meta ~fname)) because even without name, meta persisted well.


(defmacro defcached-cache [defcache-fn]
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit weird name, and I don't fully understand what's the use-case
but the use-case if probably related to the way we store meta - on function object or var ref, which adds more confusion 😞

`(cached-cache (var ~defcache-fn)))

24 changes: 16 additions & 8 deletions test/fmnoise/coldbrew_test.clj
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
(ns fmnoise.coldbrew-test
(:require [clojure.test :refer [deftest is]]
[fmnoise.coldbrew :refer [defcached cached]]))
[fmnoise.coldbrew :refer [defcached cached] :as cb])
(:import (com.github.benmanes.caffeine.cache Cache)))

(deftest cached-fn-test
(let [counter (atom 0)
Expand All @@ -14,6 +15,9 @@
c3 @counter
r4 (cf 2 3)
c4 @counter]
(is (and (instance? Cache (::cb/cache (meta cf)))
(instance? Cache (cb/cached-cache cf)))
"meta attached correctly")
(is (= 3 r1 r2 r3) "function produces correct result")
(is (= 1 c1 c2) "counter is affected only on first run")
(is (= 2 c3) "counter is affected after expiration")
Expand Down Expand Up @@ -71,14 +75,14 @@
(is (= 0 @store) "function body isn't called"))))

(deftest defcached-declaration-test
(defcached f1 [a b]
^{:expire 10} [a b]
(- a b)
(+ a b))
(defcached ^:test-meta f1 [a b]
^{:expire 10} [a b]
(- a b)
(+ a b))
(defcached f2 "adds a and b" [a b]
^{:expire 10} [a b]
(- a b)
(+ a b))
^{:expire 10} [a b]
(- a b)
(+ a b))
(defcached f3 [a b]
{:pre [(pos? a) (pos? b)]}
^{:expire 10} [a b]
Expand All @@ -89,6 +93,10 @@
^{:expire 10} [a b]
(- a b)
(+ a b))
(is (and (:test-meta (meta #'f1))
(instance? Cache (::cb/cache (meta #'f1)))
(instance? Cache (cb/defcached-cache f1)))
"meta attached correctly")
(is (= 3 (f1 1 2) (f2 1 2) (f3 1 2) (f4 1 2)) "function produces correct result")
(is (= "adds a and b" (:doc (meta #'f2)) (:doc (meta #'f4))) "docstring is added to function meta")
(is (thrown? AssertionError (f3 0 0)) "pre-conditions are added to function")
Expand Down