Skip to content

[Cache] add array cache in front of serializer cache #35046

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Cache] add array cache in front of serializer cache #35046

wants to merge 1 commit into from

Conversation

bastnic
Copy link
Contributor

@bastnic bastnic commented Dec 19, 2019 •
edited
Loading

Q A
Branch? master
Bug fix? performance
New feature? ...
Deprecations? no
Tickets #35041
License MIT
Doc PR

Protect serializer apcu/opcache backend with an array cache. It applies only in prod mode.

image

@nicolas-grekas nicolas-grekas changed the title #35041: add array cache to system cache in prod [Cache] add array cache in from of to system caches Dec 19, 2019
@nicolas-grekas nicolas-grekas changed the title [Cache] add array cache in from of to system caches [Cache] add array cache in front of to system caches Dec 19, 2019
@nicolas-grekas nicolas-grekas changed the title [Cache] add array cache in front of to system caches [Cache] add array cache in front of system caches Dec 19, 2019
@nicolas-grekas nicolas-grekas added this to the next milestone Dec 19, 2019
@bastnic
Copy link
Contributor Author

bastnic commented Dec 19, 2019

I tried to chain the system cache provider with an array adapater but it's not possible to include a chain in a chain (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/DependencyInjection/CachePoolPass.php#L119)

@nicolas-grekas
Copy link
Member

it's not possible to include a chain in a chain

Alternatively, we could remove this restriction, by merging two chained chains into one.

@nicolas-grekas
Copy link
Member

Alternatively, we could remove this restriction, by merging two chained chains into one.

forget about this one, we should "just" not tag the decorating chain with cache.pool and that would work.

@bastnic bastnic changed the title [Cache] add array cache in front of system caches [Cache] add array cache in front of serializer cache Dec 19, 2019
@bastnic bastnic force-pushed the feature/35041-cache-CacheClassMetadataFactory branch 2 times, most recently from d253cb6 to 910198e Compare December 19, 2019 14:17
@bastnic
Copy link
Contributor Author

bastnic commented Dec 19, 2019

@nicolas-grekas done.

nicolas-grekas
nicolas-grekas approved these changes Dec 19, 2019
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Extra!

@stof
Copy link
Member

stof commented Dec 19, 2019

should we do the same for some other system caches (annotations for instance) ?

@bastnic
Copy link
Contributor Author

bastnic commented Dec 19, 2019

image

So the chain is PhpArrayAdapter => ChainAdapter => (ArrayAdapter | ApcuAdapter). Not quite the direct access of a local cache.

@nicolas-grekas
Copy link
Member

should we do the same for some other system caches (annotations for instance)?

I think we should first ensure the other system caches would benefit from it. Maybe it's not worth it because the annotation cache is behind another cache? Maybe it is. It should be investigated case by case IMHO.

@bastnic bastnic force-pushed the feature/35041-cache-CacheClassMetadataFactory branch from 910198e to f7ae68e Compare December 21, 2019 17:15
@bastnic
Copy link
Contributor Author

bastnic commented Dec 21, 2019

hmm, new implementation: the array adapter was behind phparray adapter, so not useful in case of warmup. It's now in front of phparray adapter.

Still 9ms slower than a direct local cache in CacheClassMetadataFactory, with a penalty of 5ms for the chain adapter only.

Two better solutions:

  • local cache in CacheClassMetadataFactory
  • bypass chain adapter and let array adapter get a pool adpater, as phparray adatper.

@bastnic bastnic mentioned this pull request Dec 21, 2019
Merged
@bastnic
Copy link
Contributor Author

bastnic commented Dec 21, 2019

Added a faster implantation with #35079.

@xabbuh
Copy link
Member

xabbuh commented Dec 23, 2019

Should we close in favour of #35079?

@bastnic
Copy link
Contributor Author

bastnic commented Dec 23, 2019

I think we can but it is a GREAT patch to apply to any 4.x and 5.0 symfony project, as master is now 5.1 and so, release in may.

@bastnic bastnic closed this Dec 23, 2019
nicolas-grekas added a commit that referenced this pull request Dec 26, 2019
@nicolas-grekas
minor #35079 [Serializer] add array cache in front of serializer cach…
cdaebf6 
…e (bastnic)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Serializer] add array cache in front of serializer cache

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | performance
| New feature?  | ...
| Deprecations? | no
| Tickets       | #35041
| License       | MIT
| Doc PR        |

This is another implementation of #35041, after #35046.

The DI solution is cleaner, but 10ms slower on my scenario (13 vs 3) and this data is not supposed to change at all (even in the original method there is a local cache).

Diff from master (without warmup):

![image](https://user-images.githubusercontent.com/84887/71313592-2cb5be00-243b-11ea-8983-b30abb7d0698.png)

Diff form master (with warmup):

![image](https://user-images.githubusercontent.com/84887/71313633-8e762800-243b-11ea-8044-003c810d3c0b.png)

Diff from #35046:

![image](https://user-images.githubusercontent.com/84887/71313668-312ea680-243c-11ea-9768-8531f4f33f3c.png)

Ping @nicolas-grekas

Commits
-------

6ee306b [Cache] add array cache in front of serializer cache
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@lyrixx lyrixx lyrixx approved these changes

Assignees
No one assigned
Labels
Cache Performance Status: Reviewed
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
6 participants
@bastnic @nicolas-grekas @stof @xabbuh @lyrixx @carsonbot