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

[Cache] Add optimized FileSystem & Redis TagAware Adapters #30370

Merged
merged 1 commit into from Apr 24, 2019

Conversation

Projects
None yet
8 participants
@andrerom
Copy link
Contributor

commented Feb 24, 2019

Q A
Branch? master
Bug fix? no
New feature? yes TODO: src/**/CHANGELOG.md
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #28250
License MIT
Doc PR symfony/symfony-docs#... TODO

Reduces cache lookups by 50% when using TagAware, by changing logic of how tag information is stored to avoid having to look it up on getItem(s) calls.

For Filesystem symlinks are used, for Redis "Set" datatype is used.

@andrerom andrerom force-pushed the andrerom:tagaware_adapters branch from 42588d7 to eda0753 Feb 24, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Thanks for pushing this forward.
I'm wondering: does the filesystem storage benefit from the change in terms of performance?
Can you post a blackfire comparison maybe for both adapters?

@nicolas-grekas nicolas-grekas added this to the next milestone Feb 25, 2019

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

I'm wondering: does the filesystem storage benefit from the change in terms of performance?
Can you post a blackfire comparison maybe for both adapters?

It does make a noticeable difference. If you still want blackfire profile I can look into rebuilding docker env for that, but even just with web debug toolbar; if we take a demo install of eZ Platform EE (v2, Symfony 3.4LTS, dev mode, using server:start), without some other ongoing optimizations:

  • Default "cache.app" which uses files system: ~950ms total time, Cache calls 6078 in ~310ms
  • FileSystemTagAwareAdapter: ~730ms total time, Cache calls 3048 in ~135ms

Those 6078 cache calls results in 17,639 filesystem stat calls:

  • Where 11,462 are cache misses (tags with no expiry yet), which PHP does not have stat cache for
    • => These are eliminated completely with FileSystemTagAwareAdapter
  • And hits, unlike Opcache or APC cache, results in a actual disk read with fopen once initial cached stat call with file_exists() is done (however fixing that is outside of scope here)

On Redis the benefit is larger, taking the total time down from 3 to 2 seconds (and further down to <1sec with other optimizations on eZ side), where about 1.5-2 seconds are spent connecting Redis (Redis as a local docker container over TCP using Predis). I expect Redis over network to produce bigger difference.

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Tests added, both optimized TagAware Adapters fail on these:

  • ::testTagsAreCleanedOnSave
  • :::testTagsAreCleanedOnDelete

Edit: I’ll add fixes for those two failures, shouldn’t be to hard, might allow me to cleanup some things here while at it.

@nicolas-grekas What are you missing here before this can be moved out of draft besides that?

@andrerom andrerom marked this pull request as ready for review Mar 8, 2019

@andrerom andrerom force-pushed the andrerom:tagaware_adapters branch 2 times, most recently from b68ec19 to aeed059 Mar 8, 2019

@flaushi

This comment has been minimized.

Copy link

commented Mar 24, 2019

I'd like to test this PR in my project (currently on 4.2.4). I do not get how to select this PR (what is the branch name??) in my composer.json (or symfony.lock) file. Can you help me ?

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@flaushi 👍you'll need to add something like this in composer.json:

{
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/andrerom/symfony"
        }
    ],
    "require": {
        "symfony/symfony": "dev-tagaware_adapters as 4.3.x-dev"
    }
}
@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

@andrerom Any news on this one? How can we move forward?

@andrerom andrerom force-pushed the andrerom:tagaware_adapters branch 2 times, most recently from d95bf4a to 8c83bf4 Apr 7, 2019

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

@fabpot / @nicolas-grekas / @OskarStark Cache tests passing. I found a way to simplify doSave() approach, so this is ready for review from my side now.

@OskarStark
Copy link
Contributor

left a comment

@dmaicher, maybe this pull request could be interesting for you!

@andrerom andrerom force-pushed the andrerom:tagaware_adapters branch from 347d6c9 to d940c07 Apr 7, 2019

@nicolas-grekas
Copy link
Member

left a comment

That's solid work, thanks for digging into the component.
Here are some CS-related comments and then good to merge on my side.

@Toflar

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Thanks for working on this @andrerom, awesome stuff! I'd like to ask for a bit more detail regarding the implementation. So as far as I could see from a quick review, these are additional tag aware adapters alongside the existing TagAwareAdapter, right?
So I have a few questions here:

  • Nobody will benefit from these changes by just updating to the new version. Did you think about an alternative? Were there some blockers we don't know about?
  • You've introduced a new AbstractTagAwareAdapter but the existing TagAwareAdapter does not use this. Seems a bit strange to me, any reason for this?

Also in general: I'm not sure about the support for predis/predis (see nrk/predis#524). At least the decision should be in line with the Redis support for the Messenger component (#30917) where there's currently no predis/predis support planned (also likely not going to happen because it needs at least version 5 of Redis and predis/predis is not compatible and we don't know if it ever will be).

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@Toflar I can answer your question I think:

Nobody will benefit from these changes by just updating to the new version. Did you think about an alternative? Were there some blockers we don't know about?

The code here is a totally new strategy to store the tags. It does not replace the existing one but completes it when one wants to use it. Both have valid use cases, with advantages and drawbacks.

TagAwareAdapter does not use AbstractTagAwareAdapter

sure, it shouldn't: TagAwareAdapter invalidates tags using versioning, and AbstractTagAwareAdapter is the base for adapters that invalidate using relations.

Also in general: I'm not sure about the support for predis/predis

support for predis is already backed into the component since a long time. Messenger is just catching up, and anyway, it's a separate component, with its own need (eg v5)

@Toflar

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Very good, all questions answered. Then it's just a matter of documentation so that people choose the right strategy. I'll happily help out writing them 😄 👍

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Updated, all review points fixed and back to green.

@andrerom andrerom force-pushed the andrerom:tagaware_adapters branch from 2e33716 to 13f0062 Apr 15, 2019

@andrerom andrerom force-pushed the andrerom:tagaware_adapters branch from 0a2ab79 to 4db029f Apr 15, 2019

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@nicolas-grekas two things I'd like to add here:

  1. Handle case where people switch adapters. Suggested handling for going from plain to TagAware in ddac001, but maybe we should handle other way around also?
  2. Consider that we should throw on invalidation if Redis server is older then 3.2, to make people more aware of requirements. If so I'll want to be able to reuse get hosts logic in RedisTrait::doClear by moving it to own method.
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Handle case where people switch adapters

The logic looks great for robustness - there is a missing \is_array($value) somewhere btw, isn't it?
An additional measure could be to generate a different namespace. CachePoolPass::getNamespace() could add the class of the adapter to the hash, don't you think?

Consider that we should throw on invalidation if Redis server is older then 3.2

would be a contracts break - invalidateTags() should return false and when a logger is provided to the adapter, this should be logged for sure

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

The logic looks great for robustness - there is a missing \is_array($value) somewhere btw, isn't it?
An additional measure could be to generate a different namespace. CachePoolPass::getNamespace() could add the class of the adapter to the hash, don't you think?

that would be an alternative, and probably cleaner. Any prefs besides what you wrote?
This is actually not an issue for FilesystemAdapters as they use static::class in getFile()

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Consider that we should throw on invalidation if Redis server is older then 3.2

would be a contracts break - invalidateTags() should return false and when a logger is provided to the adapter, this should be logged for sure

hmm, silently fail then. Maybe this is a feature request to requirements handling for symfony, to not have to look that up on runtime?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

any prefs?

Doing both :)

silently fail then.

The constructor can throw

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Doing both :)

ok with the namespace thing as separate PR?

The constructor can throw

I'd rather not do the requirement check with server lookup on every request, this is why I proposed to align with doClear and do it on demand on call to doInvalidate().

@nicolas-grekas
Copy link
Member

left a comment

Lasts comments I suppose :)

@andrerom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

@nicolas-grekas Updated

EDIT: Added test for the namespace being seeded by adapter name.

@@ -46,9 +42,13 @@ protected function __construct(string $namespace = '', int $defaultLifetime = 0)
function ($key, $value, $isHit) use ($defaultLifetime) {
$item = new CacheItem();
$item->key = $key;
$item->defaultLifetime = $defaultLifetime;
// If structure is in TagAware format, return as is (no value and not a hit)
if (\is_array($value) && \array_key_exists('value', $value)) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 22, 2019

Member

I'm really unsure about this check: this doesn't look specific enough to me. Not sure we need to do anything IMHO.

This comment has been minimized.

Copy link
@andrerom

andrerom Apr 23, 2019

Author Contributor

Not that we have namespace separation we can drop it

if ($t = $adapter->getTag($this->cachePoolTag)) {
$tags[0] += $t[0];
}
}
$name = $tags[0]['name'] ?? $id;
if (!isset($tags[0]['namespace'])) {
$tags[0]['namespace'] = $this->getNamespace($seed, $name);
$tags[0]['namespace'] = $this->getNamespace($seed, $name.$class);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 22, 2019

Member

I'd suggest a separator here, between the name and the class, just in case.

This comment has been minimized.

Copy link
@andrerom

andrerom Apr 23, 2019

Author Contributor

Using which character?

This comment has been minimized.

Copy link
@nicolas-grekas

@nicolas-grekas nicolas-grekas force-pushed the andrerom:tagaware_adapters branch from 96db3cf to 098a6ea Apr 24, 2019

@nicolas-grekas
Copy link
Member

left a comment

I just push-forced the fix for my last comments, all good on myside thank you!

@nicolas-grekas nicolas-grekas force-pushed the andrerom:tagaware_adapters branch from 098a6ea to 412167a Apr 24, 2019

[Cache] Add optimized FileSystem & Redis TagAware Adapters
Reduces cache lookups by 50% by changing logic of how tag information is
stored to avoid having to look it up on getItem(s) calls.

For Filesystem symlinks are used, for Redis "Set" datatype is used.

@nicolas-grekas nicolas-grekas force-pushed the andrerom:tagaware_adapters branch from 412167a to 3278cb1 Apr 24, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Thank you @andrerom.

@fabpot fabpot merged commit 3278cb1 into symfony:master Apr 24, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 24, 2019

feature #30370 [Cache] Add optimized FileSystem & Redis TagAware Adap…
…ters (andrerom)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Cache] Add optimized FileSystem & Redis TagAware Adapters

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes _TODO: src/**/CHANGELOG.md_
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28250
| License       | MIT
| Doc PR        | symfony/symfony-docs#... TODO

Reduces cache lookups by 50% when using TagAware, by changing logic of how tag information is stored to avoid having to look it up on getItem(s) calls.

For Filesystem symlinks are used, for Redis "Set" datatype is used.

Commits
-------

3278cb1 [Cache] Add optimized FileSystem & Redis TagAware Adapters

@andrerom andrerom deleted the andrerom:tagaware_adapters branch Apr 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.