Skip to content
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

[Feature] Optimized TagAware Symfony Cache File & Redis Adapters #4

Merged
merged 33 commits into from Mar 11, 2019

Conversation

andrerom
Copy link
Contributor

@andrerom andrerom commented Feb 1, 2019

Reduces lookups to cache backend by half 🎉

This is done by changing tag design to not have to do seperate lookup for tag info on reads to figure out if they have expired. Instead it's modeled to handle lookup of tag info when needed on invalidation by storing tag info separately (Filesystem: symlinks, Redis: a Set with no expiry).

Further info on design in doc and phpdoc.

This is meant to be contributed to Symfony, but as we are on Symfony 3, need this asap, and as it will probably take some iterations to get it right there. We add it here first, then contribute to Symfony 4, and then adapt code here to become a backport.

Dev testing done:

  • Light manual testing with
    • Filesystem
    • Redis with Predis, but not with phpredis from my side
  • Unit / integration testing? Is there some TagAware test suites in symfony we can copy in here and run across the misc setups?

Dependency:

Side, but for the overall story; eZ specific cache/performance followups:

  • Continue on new Bulk API loading improvements, as in take advantage in admin, page builder, .. to reduce number of lookups/roundtrips. +Document how end users should do the same
  • Open kernel PR for in-memory SPI caching for v2.5
  • Further profiling to identify remaining bottlenecks planned for March by engineering

Reduces lookups to cache backend by half, by changing tag design to not
have to do seperate lookup for tag info on reads. Instead
model to handle lookup when needed on invlidation.

Further info on design in doc and phpdoc.
Copy link
Member

@kmadejski kmadejski left a comment

Choose a reason for hiding this comment

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

Besides mentioned below, I'm getting the following PHP's fatal error while trying to clear the cache:

PHP Fatal error:  Uncaught Error: Call to undefined method Symfony\Component\Cache\Adapter\TraceableTagAwareAdapter::__destruct() in /Users/kamil/ez/var/cache/dev/ContainerKgikhg8/TagAwareAdapter_5b4740c.php:95
Stack trace:
#0 [internal function]: TagAwareAdapter_5b4740c->__destruct()
#1 {main}
  thrown in /Users/kamil/www/ez/var/cache/dev/ContainerKgikhg8/TagAwareAdapter_5b4740c.php on line 95

but the cache clearing finishes successfully and the application works fine. Not everything. Investigating.

}
});

$ids = array_unique(array_merge(...$tagIdSets))

This comment was marked as resolved.

This comment was marked as resolved.

$item = new CacheItem();
$item->key = $key;
//<diff:AbstractAdapter> extract Value and Tags from the cache value
$item->value = $value['value'];
Copy link
Member

@kmadejski kmadejski Feb 8, 2019

Choose a reason for hiding this comment

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

Apparently, the index value may not exists if you are about to use this Adapter without clearing the cache before.

Suggested change
$item->value = $value['value'];
$item->value = $value['value'] ?? null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you have not cleared cache it will not be an array and you will be missing info on tags.

So maybe we need to throw exception if value is missing saying you need to clear cache after enabling the adapter ;)

// Retrive and delete items in bulk of 10.000 at a time to not overflow buffers
$getId = $this->getId;
do {
$tagIdSets = $this->pipeline(function () use ($tags, $getId, $limit) {

This comment was marked as resolved.

This comment was marked as resolved.

}
});

$ids = array_unique(array_merge(...$tagIdSets));

This comment was marked as resolved.

This comment was marked as resolved.

@andrerom andrerom changed the title [Feature] Optimized TagAware Symfony Cache File & Redis Adapters [WIP][Feature] Optimized TagAware Symfony Cache File & Redis Adapters Feb 11, 2019
@andrerom andrerom changed the title [WIP][Feature] Optimized TagAware Symfony Cache File & Redis Adapters [Feature] Optimized TagAware Symfony Cache File & Redis Adapters Feb 12, 2019
@andrerom
Copy link
Contributor Author

andrerom commented Feb 12, 2019

PHP Fatal error: Uncaught Error: Call to undefined method Symfony\Component\Cache\Adapter\TraceableTagAwareAdapter::__destruct()

This one should have been fixed in: ezsystems/ezpublish-kernel@548f7d6

@andrerom andrerom force-pushed the native_tagaware_cache branch 2 times, most recently from a8b3032 to f378730 Compare February 25, 2019 12:51
@andrerom
Copy link
Contributor Author

andrerom commented Feb 25, 2019

@kmadejski PR updated now after PR with these adapters was opened on Symfony.

Some of things ended up being possible to simplify, including ended up getting some performance issues for RedisCluster on fetch logic merged to 3.4, which further helped simplify things here by getting rid of some of the overloading needs.

@kmadejski

This comment has been minimized.

@andrerom
Copy link
Contributor Author

@kmadejski suggestions for second person for review?

@kmadejski
Copy link
Member

kmadejski commented Feb 28, 2019

Honestly, all good candidates which come into my mind are already on the list. Maybe @vidarl or @dspe could take a look as well?

@andrerom
Copy link
Contributor Author

@kmadejski Besides missing links in php doc for source, could you review?

REVIEW NOTE: I ended up back-porting a few more classes/traits from Symfony 4 to fix RedisCluster things, but intention is to try to propose fixes to Redis Cluster on 3.4 to back-port some smaller fixes there so RedisTrait and RedisClusterProxy can be removed from this repo.

@andrerom andrerom merged commit f9f4cc9 into master Mar 11, 2019
@andrerom andrerom deleted the native_tagaware_cache branch March 11, 2019 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants