https://www.drupal.org/node/2056373 Added basic cache tag support, using the database tag service. This covers most of what we need. There are still issues as we cannot invalidate or delete a whole bin properly yet. Without nuking everything.

A couple of options:

- Similar to 7.x (probably favourable), store a timestamp, probably in state, for the last invalidated or deleted times. Check this when determining if an item(s) is valid. Could apply to both invalidateAll and deleteAll. See below...
- Have a generated prefix that we just regenerate when deleteaAll() is used.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mcdruid’s picture

Status: Active » Needs review
FileSize
2.04 KB

Here's a very rough patch which implements a randomly generated key pre/suffix per bin, which we then regenerate when invalidateAll() is called.

mcdruid’s picture

FileSize
2.03 KB

Tried a few different ways of using state properly (e.g. injecting it in services) but that seems to lead to "Circular reference" issues.

Here's an alternative approach; using memcache itself to store the key suffixes. Seems quite a bit simpler, and AFAICS it works.

MiSc’s picture

Priority: Critical » Normal

Seems like a good approach @mcdruid if no one else have time to review this I try do it the coming week.

webchick’s picture

webchick’s picture

Issue tags: +beta blocker

Ahem.

damiankloip’s picture

FileSize
6.41 KB

Here is an extended patch for this, the original approach will work for deletions but invalidations, not so much. I.e. requesting an item with $allow_invalid will not work with this approach. So I think we need a way to do prefixing (for deletions) but also invalidations only, with a microtime per bin.

damiankloip’s picture

Priority: Normal » Critical

Assigning back to critical, this is completely missing functionality from the cache backend.

damiankloip’s picture

FileSize
5.53 KB
8.01 KB

This approach might be better, using cache tags for invalidations, and using a timestamp for delete all time.

We can then utilise new services for both cache tags and delete timestamp prefixing in the future to improve the stability of this.

Fabianx’s picture

Overall looks almost RTBC.

  1. +++ b/src/MemcacheBackend.php
    @@ -1,13 +1,8 @@
    -use Drupal\Core\Cache\Cache;
    +use Drupal\Component\Utility\Crypt;
    

    no longer needed

  2. +++ b/src/MemcacheBackend.php
    @@ -316,7 +325,62 @@ class MemcacheBackend implements CacheBackendInterface {
    +  protected function timeIsGreaterThanBinDeletionTime(float $item_microtime) {
    ...
    +  protected function getBinLastDeletionTime() {
    ...
    +  protected function updateBinLastDeletionTime() {
    ...
    +  protected function ensureBinDeletionTimeIsSet() {
    ...
    +  protected function getBinLastDeletionTimeKey() {
    

    Let's mark all of that as @internal.

    I have not given up hope to use an injected timestamp service instead ;).

  3. +++ b/src/MemcacheBackend.php
    @@ -316,7 +325,62 @@ class MemcacheBackend implements CacheBackendInterface {
    +    if (!$last_bin_deletion) {
    +      return FALSE;
    +    }
    

    Should create the last_bin_deletion time here - though it's really unlikely we would not have one here as we do it on __construct(), so likely not needed.

  4. +++ b/src/MemcacheBackend.php
    @@ -316,7 +325,62 @@ class MemcacheBackend implements CacheBackendInterface {
    +    $microtime = microtime(TRUE);
    +
    +    $return = $this->memcache->set($this->getBinLastDeletionTimeKey(), $microtime);
    +
    +    $this->lastBinDeletionTime = $microtime;
    

    Are we sure we want to use microtime for this?

    Usually timedrift can mean cache corruption, but using normal seconds we just loose some data.

    There was a patch in the D7 queue on that to convert to microtime [which gave horrible effects], so I am tending to use normal seconds instead.

Fabianx’s picture

As discussed we'll continue to use microtime() rounded plus some skew like ChainedFast.

As we are dealing with volatile storage, which can evict keys without notice, we can't rely on atomic increment / decrement behavior unfortunately.

catch’s picture

This is the hunk from ChainedFastBackend for reference:

  protected function markAsOutdated() {
    // Clocks on a single server can drift. Multiple servers may have slightly
    // differing opinions about the current time. Given that, do not assume
    // 'now' on this server is always later than our stored timestamp.
    // Also add 1 millisecond, to ensure that caches written earlier in the same
    // millisecond are invalidated. It is possible that caches will be later in
    // the same millisecond and are then incorrectly invalidated, but that only
    // costs one additional roundtrip to the persistent cache.
    $now = round(microtime(TRUE) + .001, 3);

Approach here looks right to me, agreed with tags for invalidations and state for deletions.

damiankloip’s picture

Yes, the skew is a good idea here. We talked about all the other stuff too! Here is a patch which I think is good now.

vurt’s picture

#12 applied fine against the current version.

With PHP 5.6 I get the following error:

PHP Catchable fatal error:  Argument 1 passed to Drupal\memcache\MemcacheBackend::timeIsGreaterThanBinDeletionTime() must be an instance of Drupal\memcache\float, double given, called in MemcacheBackend.php on line 109
if (!$this->timeIsGreaterThanBinDeletionTime($result->created)) {
...
protected function timeIsGreaterThanBinDeletionTime(float $item_microtime) {

When I remove the float it seems to work fine.

A crude performance test with "time drush cr" shows that the new version is about 10% slower than the old one (with global flush).

Thanks for the work! I am happy to see that memcache is moving forward.

damiankloip’s picture

FileSize
5.91 KB
671 bytes

Thanks for testing vurt! Very useful.

It's a good point about the float typehinting. I forget that we still support PHP5.6 sometimes :)

I think the additional time is ok, as before everything was just doing a memcache flush() directly.

Having this functionality in is important, so going to commit this. We can follow up anything related from here.

damiankloip’s picture

ehm ,this.

  • damiankloip committed 2d0b1a0 on 8.x-2.x
    Issue #2484595 by damiankloip, mcdruid, Fabianx, vurt, catch: Add...
damiankloip’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.