Conversation
src/ArrayCache.php
Outdated
| $this->data[$key] = [ | ||
| 'value' => $value, | ||
| 'expires' => $expires, | ||
| ]; |
There was a problem hiding this comment.
Not a big fan of mixing these data structures. Probably makes more sense to store expiration in a separate array or possibly SplPriorityQueue?
src/ArrayCache.php
Outdated
|
|
||
| if (isset($this->expires[$key]) && $this->expires[$key] < time()) { | ||
| unset($this->data[$key], $this->expires[$key]); | ||
| return Promise\resolve(); |
There was a problem hiding this comment.
This should be updated to take advantage of #27? Also applies to documentation.
src/CacheInterface.php
Outdated
| * | ||
| * @param string $key | ||
| * @param mixed $value | ||
| * @param int|null $ttl |
There was a problem hiding this comment.
Should this use float instead of int seconds?
There was a problem hiding this comment.
If storing things in a cache for sub second times then yes, not sure how often that happens though
There was a problem hiding this comment.
Admittedly, my use cases would be fulfilled by having just second precision.
However, at the very least this PR would have to be updated to use microtime(true) instead of time() everywhere, so that storing an item at 09:49.999 with 2s TTL would expire at 09:51.999 instead of 09:51.000 already.
At this point I would argue we might as well accept a float TTL (also for consistency with react/event-loop). But given I don't really have a use case that requires this right now, I guess I'm okay with this either way 🤷♂️
There was a problem hiding this comment.
Ok changed it to float and microtime(true)
src/ArrayCache.php
Outdated
| reset($this->data); | ||
| unset($this->data[key($this->data)]); | ||
| $key = key($this->data); | ||
| unset($this->data[$key], $this->expires[$key]); |
There was a problem hiding this comment.
This will need to check if any items are expired first and then remove this one. Only if no expired items can be found, it should overwrite the oldest item (LRU from #26).
Right now, this may overwrite an item that an item that is not expired already.
For practical reasons it may overwrite any expired item. We can either loop over this array with O(n) or use a SplPriorityQueue with O(log n) which would make this faster for larger arrays (premature optimization?).
There was a problem hiding this comment.
Good point I'll fix that 👍 .
I'de vote for making it working now and do the I'll try out both to see which turns out to be the simplest to implement and read.SplPriorityQueue optimization in a follow up PR with benchmarks to see how big the difference is for different cache sizes.
There was a problem hiding this comment.
Took me a bit, but got it to work with SplPriorityQueue pushing soon 👍
|
@clue updated to both your suggestions. |
2 participants
Implements a TTL as RFCd in #11.
Implements / Closes #11