Skip to content

[Cache] Decrease the probability of invalidation loss on tag eviction #44015

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 5.4
Bug fix? no
New feature? no
Deprecations? no
Tickets Fix #43301
License MIT
Doc PR -

From @sbelyshkin in the linked PR:

When using TagAwareAdapter with volatile caches such as Memcached and Redis with 'alkeys-lru' eviction policy, eviction of any key may happen.

Currently, tag-based invalidation is achieved by incrementing tag version numbers starting from 0. The 0th version is assumed by default and is not even stored in the cache, only non-zero versions are stored. Because of that, when a tag version key is evicted from cache, version counter is "reset" to 0, and if there are items still stored with tag version 0 (had not been accessed/updated by the moment of tag eviction), such items become valid again while must be deemed invalidated. Similarly invalidation may be lost when tag is invalidated particular number of times after its eviction (for items with non-zero tag version).

This scenario is more likely for item-tag pairs which are accessed and invalidated less frequently than others (due to LRU policy and lower version numbers) but this measure is quite relative. LRU caches implement different algorithms (e.g. https://redis.io/topics/lru-cache, https://memcached.org/blog/modern-lru/) and free to evict any key of their choice so even a tag which is accessed or invalidated pretty frequently can be evicted when LRU cache is full and under havy loads.

In order to prevent invalidation losses, any non-repetative numbers can be used as initial value for tag versions.

This PR goes one step further and borrows from #42997: instead of incrementing the version number on invalidation, we delete the value from the backend, and recreate it when needed with a random offset.

This PR also removes auto-commit on tag-invalidation: this behavior adds complexity, is not needed and is not consistent with Redis|FilesystemTagAwareAdapter.

/cc @sbelyshkin can you please give it a go in a review?

@carsonbot
Copy link

Hey!

I think @fancyweb has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

if (isset($invalidatedTags[$tag])) {
$invalidatedTags[$tag] = $version->set(++$tagVersions[$tag]);
if (!$version->isHit()) {
$newTags[] = $version->set($newVersion ?? $newVersion = random_int(\PHP_INT_MIN, \PHP_INT_MAX));
Copy link
Contributor

Choose a reason for hiding this comment

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

mt_rand() is faster than random_int() and gives the same level of randomness

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer keeping random_int(): the speed difference is negligible compared to network round-trips, and it cannot be seeded, while mt_rand() can, with bad side effects for our use case

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree.

Copy link
Contributor

@sbelyshkin sbelyshkin left a comment

Choose a reason for hiding this comment

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

Looks good.

@fabpot
Copy link
Member

fabpot commented Nov 16, 2021

Thank you @nicolas-grekas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants