Open jobh opened 1 week ago
If you dislike the new pin
semantics, a fairly straightforward alternative is to allow "sticky" pinning of unknown keys by storing them as not_set
. I.e., it's the key that's pinned, even if it's not set; and the entry will start out pinned if it's added later. I see some beauty in this approach, but it's perhaps a bit unconventional.
Unfortunately it might be a week or two before I have space to reload context and review this properly, but if e.g. @tybug wants to review this I'm happy to confirm and merge.
(also, fantastic that we have coverage again - it's already catching an untested branch 🥳)
will check this in the next few days!
Am I correct in understanding that with the new pin semantics, if we pin a key that has been evicted (and don't pass value
), we will get a KeyError
? This feels like a bit of a footgun.
KeyError
when pinning an evicted keyunpin
it you get a surprise errorKeyError
under cache pressureI would say your proposed sticky semantics solves these concerns and would actually advocate for that. It makes me a bit nervous in its fanciness but I can't think of a failure case?
Other changes look good 👍. I added one comment where I had to stop and think about a condition, but feel free to remove if you think it's clear without.
Thanks for the review @tybug!
- old footgun: if a key is evicted before you pin it, and you later re-cache that key, you think it's pinned but the cache doesn't, so if you
unpin
it you get a surprise error- new footgun: if you forget to use the atomic option, or can't because you only have the key, you may get a fatal
KeyError
under cache pressure
Yes, this is correctly understood, and a nice explanation of the problem.
I pushed a change now to just require the value to be supplied to pin()
. It is a bit unsatisfying, being less orthogonal and more repetitive API-wise. But on balance I think it's wiser than the sticky semantics, they do add a little complexity which we don't really need in our internal usage (we always have the value).
(I have implemented the sticky semantics too, so it's no extra work to switch tough)
Note, I added a minor change to always compare sort_key
instead of score
, and to remove a bad comment and associated no-cover pragma.
Note, I added a minor change to always compare
sort_key
instead ofscore
, and to remove a bad comment and associated no-cover pragma.
It turned out this was a real bug (heap-property violation), so I spent some more time understanding and fixing the heap balancing. The stateful test now checks the heap property at each step, so I'm fairly confident about the change. Sorry for doing this post-review.
Fix some issues with the cache implementation.
No actual bugs, only latent issues and cleanup.on_access
should return the updated score, but the code expected the score to be modified in-place. Code changed to match docs.pin()
on expired keys" is unsafe: if we later insert the key, the pin-state of the key is unknown. This can be observed "in the wild" in our current usage — by adding artificial cache pressure beforepin()
we fail when callingunpin()
on a not-pinned key.max_size=0
(ignoring new entries) is no longer allowed, as that violates the handy (for atomicity) "last entry successfully set is present in cache" invariant.[edit]
score
instead ofsort_key