[Cache] Add stampede protection via probabilistic early expiration #27009
Conversation
8e556be to
0b647d2
Compare
| */ | ||
| public function get(string $key, callable $callback, float $beta = null) | ||
| { | ||
| if (!\is_string($key)) { |
There was a problem hiding this comment.
Just asking: above we have string $key in the method argument ... will this if (!is_string($key)) code be ever executed if we pass a non-string argument?
| ); | ||
| $this->setInnerItem = \Closure::bind( | ||
| function (CacheItemInterface $innerItem, array $item) { | ||
| if ($stats = $item["\0*\0newStats"]) { |
There was a problem hiding this comment.
Not sure it it'd help much, but maybe move \0*\0 to private const PREFIX = '\0*\0'; in this class?
There was a problem hiding this comment.
Not sure also, using the const would just add indirection IMHO, for a purely internal thing.
Other comments addressed thanks.
| { | ||
| /** | ||
| * @param callable(CacheItemInterface $item):mixed $callback Should return the computed value for the given key/item | ||
| * @param float $beta A float that controls the likelyness of triggering early expiration. |
| /** | ||
| * @param callable(CacheItemInterface $item):mixed $callback Should return the computed value for the given key/item | ||
| * @param float $beta A float that controls the likelyness of triggering early expiration. | ||
| * 0 disables it, INF forces immediate expiration. The default is implementation dependend |
| final class CacheItem implements CacheItemInterface | ||
| { | ||
| /** | ||
| * References the unix timestamp stating when the item will expire, as integer. |
c2e16bf to
6ae48dc
Compare
|
ping @Nyholm FYI: here, I deprecate getPreviousTags() and replace it by getMetadata() |
7a76d9f to
af895ff
Compare
af895ff to
aca75fa
Compare
I designed it this way so that the interface is generic enough. Hardcoding 1.0 as default would prevent any other implementations from providing a different default. Yes, there are no other implementations than the ones in this PR, but that's not a reason to break the abstraction provided by the interface. |
99634fc to
472b593
Compare
| { | ||
| /** | ||
| * @param callable(CacheItemInterface $item):mixed $callback Should return the computed value for the given key/item | ||
| * @param float|null $beta A float that controls the likeness of triggering early expiration. |
There was a problem hiding this comment.
I did the same mistake as you, but @javiereguiluz made me fix it: it's really "likeness".
There was a problem hiding this comment.
They have different meanings. You mean likeliness here.
likeliness = probability
likeness = resemblance
There was a problem hiding this comment.
indeed, sorry for the misunderstanding (on my side)! (fixed)
5e7db51 to
d01a58b
Compare
d01a58b to
5076b6d
Compare
|
Rebased, PR ready for review. |
5076b6d to
7ec3d50
Compare
|
PR is ready. The protection needs storing expiry+computation time. This is done by encoding these values in the key of a wrapping array: by using this magic-numbered structure, we are able to persist both raw + wrapped values in the same store, providing forward/backward compat at the storage level. Stampede protection is always enabled when accessing values through the new |
|
Does the |
|
That's the thing we need to discuss :) |
a81d3ba to
fdce538
Compare
|
Rebased |
|
ping @symfony/deciders |
| $item->value = $v = $value; | ||
| $item->isHit = $isHit; | ||
| $item->defaultLifetime = $defaultLifetime; | ||
| if (\is_array($v) && 1 === \count($v) && 10 === \strlen($k = \key($v)) && "\x9D" === $k[0] && "\0" === $k[5] && "\x5F" === $k[9]) { |
There was a problem hiding this comment.
This line looks really complicated. I cannot tell what it is doing. Maybe add a comment?
There was a problem hiding this comment.
Comments added, I hope it will be more clear.
| ); | ||
| $this->setInnerItem = \Closure::bind( | ||
| function (CacheItemInterface $innerItem, array $item) { | ||
| if (isset(($metadata = $item["\0*\0newMetadata"])[CacheItem::METADATA_TAGS])) { |
There was a problem hiding this comment.
Same here, Im really trying to understand this, but I can't =)
There was a problem hiding this comment.
Comment added, better?
fdce538 to
f6ca712
Compare
| $item->isHit = $isHit; | ||
| $item->defaultLifetime = $defaultLifetime; | ||
| // Detect wrapped values that encode for their expiry and creation duration | ||
| // For compacity, these values are packed in the key of an array using magic numbers |
| $item->defaultLifetime = $defaultLifetime; | ||
| // Detect wrapped values that encode for their expiry and creation duration | ||
| // For compacity, these values are packed in the key of an array using magic numbers | ||
| if (\is_array($v) && 1 === \count($v) && 10 === \strlen($k = \key($v)) && "\x9D" === $k[0] && "\0" === $k[5] && "\x5F" === $k[9]) { |
There was a problem hiding this comment.
Magic 10 here, not sure if we want to make it explicit (as we would want to make 5 and 9 explicit as well).
| if (isset(($metadata = $item->newMetadata)[CacheItem::METADATA_TAGS])) { | ||
| unset($metadata[CacheItem::METADATA_TAGS]); | ||
| } | ||
| // For compacity, expiry and creation duration are packed in the key of a array, using magic numbers as separators |
| $item->innerItem = $innerItem; | ||
| $item->poolHash = $poolHash; | ||
| // Detect wrapped values that encode for their expiry and creation duration | ||
| // For compacity, these values are packed in the key of an array using magic numbers |
f6ca712 to
fe26414
Compare
|
comments addressed thanks |
| * | ||
| * @return array | ||
| * | ||
| * @deprecated since Symfony 4.2. Use the "getMetadata()" method instead. |
fe26414 to
13523ad
Compare
|
Thank you @nicolas-grekas. |
…y expiration (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Cache] Add stampede protection via probabilistic early expiration | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | This PR implements [probabilistic early expiration](https://en.wikipedia.org/wiki/Cache_stampede#Probabilistic_early_expiration) on top of `$cache->get($key, $callback);` It adds a 3rd arg to `CacheInterface::get`: > float $beta A float that controls the likelyness of triggering early expiration. 0 disables it, INF forces immediate expiration. The default is implementation dependend but should typically be 1.0, which should provide optimal stampede protection. Commits ------- 13523ad [Cache] Add stampede protection via probabilistic early expiration
…lculations (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Cache] Use sub-second accuracy for internal expiry calculations | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | not really | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Embeds #26929, #27009 and #27028, let's focus on the 4th commit for now. This is my last significant PR in the Cache series :) By using integer expiries internally, our current implementations are sensitive to abrupt transitions when time() goes to next second: `$s = time(); sleep(1); echo time() - $s;` *can* display 2 from time to time. This means that we do expire items earlier than required by the expiration settings on items. This also means that there is no way to have a sub-second expiry. For remote backends, that's fine, but for ArrayAdapter, that's a limitation we can remove. This PR replaces calls to `time()` by `microtime(true)`, providing more accurate timing measurements internally. Commits ------- 08554ea [Cache] Use sub-second accuracy for internal expiry calculations
This PR implements probabilistic early expiration on top of
$cache->get($key, $callback);It adds a 3rd arg to
CacheInterface::get: