Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Cache] Add stampede protection via probabilistic early expiration#27009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromnicolas-grekas:cache-stats
Jun 11, 2018

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedApr 22, 2018•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR

This PR implementsprobabilistic early expiration on top of$cache->get($key, $callback);

It adds a 3rd arg toCacheInterface::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.

*/
publicfunctionget(string$key,callable$callback,float$beta =null)
{
if (!\is_string($key)) {

Choose a reason for hiding this comment

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

Just asking: above we havestring $key in the method argument ... will thisif (!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"]) {

Choose a reason for hiding this comment

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

Not sure it it'd help much, but maybe move\0*\0 toprivate const PREFIX = '\0*\0'; in this class?

Copy link
MemberAuthor

@nicolas-grekasnicolas-grekasApr 23, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Not sure also, using the const would just add indirection IMHO, for a purely internal thing.
Other comments addressed thanks.

javiereguiluz reacted with thumbs up emoji
{
/**
* @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.

Choose a reason for hiding this comment

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

likelyness ->likeness

/**
* @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

Choose a reason for hiding this comment

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

dependend ->dependent

finalclass CacheItemimplements CacheItemInterface
{
/**
* References the unix timestamp stating when the item will expire, as integer.

Choose a reason for hiding this comment

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

unix ->Unix

@nicolas-grekasnicolas-grekasforce-pushed thecache-stats branch 3 times, most recently fromc2e16bf to6ae48dcCompareApril 23, 2018 08:48
@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedApr 23, 2018•
edited
Loading

ping@Nyholm FYI: here, I deprecate getPreviousTags() and replace it by getMetadata()

@nicolas-grekasnicolas-grekasforce-pushed thecache-stats branch 2 times, most recently from7a76d9f toaf895ffCompareApril 23, 2018 19:36
@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedApr 24, 2018•
edited
Loading

I don't like unclear default values. Why not 1.0 directly?

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.
Note that PSR-6 does the same for default expiry of items.

@nicolas-grekasnicolas-grekasforce-pushed thecache-stats branch 2 times, most recently from99634fc to472b593CompareApril 24, 2018 10:07
{
/**
* @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.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: likeliness

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I did the same mistake as you, but@javiereguiluz made me fix it: it's really "likeness".

Copy link
Contributor

Choose a reason for hiding this comment

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

They have different meanings. You mean likeliness here.
likeliness = probability
likeness = resemblance

javiereguiluz reacted with thumbs up emoji
Copy link
MemberAuthor

@nicolas-grekasnicolas-grekasApr 24, 2018•
edited
Loading

Choose a reason for hiding this comment

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

indeed, sorry for the misunderstanding (on my side)! (fixed)

javiereguiluz reacted with thumbs up emoji
@nicolas-grekas
Copy link
MemberAuthor

Rebased, PR ready for review.

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedMay 28, 2018•
edited
Loading

PR is ready.

The protection needs storing expiry+computation time. This is done by encoding these values in the key of a wrapping array:
$persistedValue = array("\x9D".$expiry.$computeTime."\x5F" => $cachedValue);

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 newCacheInterface::get() method (one should use the PSR-6 interface if they don't want the overhead of storing expiry+compute-time, thus opting out from stampede protection.)

@Nyholm
Copy link
Member

Does thegetPreviousTags really need to be deprecated? Wouldn't it be good to still have that? As far as I know that would make us compatible with the "soon to be"-PSR

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedMay 28, 2018•
edited
Loading

That's the thing we need to discuss :)
So, my current pov is that accessing tags is useful, but also is accessing the expiry. One use case for the expiry is the one present in this PR, another one is for HTTP caches, allowing to compute the max-age directive when serving cached values.
Which means if we spend some effort having an updated PSR, it could be worth doing it once for all.
ThisgetMetadata() method would be my proposal: an array with keyed values, thus extensible, but still having some keys reserved by const name at least (tags, expiry - computation time could be a custom extension - or in the PSR).

@nicolas-grekas
Copy link
MemberAuthor

Rebased

@nicolas-grekas
Copy link
MemberAuthor

ping @symfony/deciders

Copy link
Member

@NyholmNyholm left a comment

Choose a reason for hiding this comment

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

I've looked at this PR. It is real hard to understand what you are doing. Even though I know what you are trying to do. :/

I do not mind the API changes but I would be happy to see some more comments.

$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]) {
Copy link
Member

Choose a reason for hiding this comment

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

This line looks really complicated. I cannot tell what it is doing. Maybe add a comment?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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])) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, Im really trying to understand this, but I can't =)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Comment added, better?

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

with minor comments

$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
Copy link
Member

Choose a reason for hiding this comment

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

for compactness?

$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]) {
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

compactness as well?

$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
Copy link
Member

Choose a reason for hiding this comment

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

same?

@nicolas-grekas
Copy link
MemberAuthor

comments addressed thanks

*
* @return array
*
* @deprecated since Symfony 4.2. Use the "getMetadata()" method instead.
Copy link
Member

Choose a reason for hiding this comment

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

4.2, use the

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

updated

@fabpot
Copy link
Member

Thank you@nicolas-grekas.

@fabpotfabpot merged commit13523ad intosymfony:masterJun 11, 2018
fabpot added a commit that referenced this pull requestJun 11, 2018
…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
fabpot added a commit that referenced this pull requestJun 11, 2018
…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
@nicolas-grekasnicolas-grekas deleted the cache-stats branchJune 11, 2018 13:38
@nicolas-grekasnicolas-grekas modified the milestones:next,4.2Nov 1, 2018
This was referencedNov 3, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz left review comments

@NyholmNyholmNyholm left review comments

@fabpotfabpotfabpot approved these changes

+1 more reviewer

@TobionTobionTobion left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

6 participants

@nicolas-grekas@Nyholm@fabpot@javiereguiluz@Tobion@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp