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

Add PhpArrayAdapter to cache metadata #1186

Closed
ossinkine opened this issue Jul 14, 2020 · 12 comments · Fixed by #1196
Closed

Add PhpArrayAdapter to cache metadata #1186

ossinkine opened this issue Jul 14, 2020 · 12 comments · Fixed by #1196

Comments

@ossinkine
Copy link
Contributor

I propose to cache metadata using PhpArrayAdapter the same way as serializer or validator in Symfony.
I've added it in my project and got 1-2% speed up on production.

If the communtity approve this, I will create a PR.

@dmaicher
Copy link
Contributor

dmaicher commented Jul 14, 2020

I think this is done already using recipes?

https://github.com/symfony/recipes/blob/master/doctrine/doctrine-bundle/1.12/config/packages/prod/doctrine.yaml#L6

And cache.system should use PhpArrayAdapter

Edit: actually it uses PhpFilesAdapter

@ossinkine
Copy link
Contributor Author

This is not entirely true. PhpArrayAdapter is not a typical adapter, you can't just set cache.adapter.php_array or something like that, there is no a such service: https://github.com/symfony/symfony/blob/5.1/src/Symfony/Bundle/FrameworkBundle/Resources/config/cache.xml.

Moreover it should be warmed up before using. Usually it's using in prod env as a decorator above configured adapter and warming up on bootstrap.

@dmaicher
Copy link
Contributor

And you say using PhpArrayAdapter would be faster than using cache.system?

@ossinkine
Copy link
Contributor Author

I used APCu and PhpArrayAdapter a little bit faster in my environment.

No wonder the Symfony uses it in prod in Validator, Serializer and Annotation components 😉

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 14, 2020

PhpArrayAdapter shouldn't be used if it cannot be warmed up, that's why it's used only for some specific use cases. The reason is that opcache (which is the backend used by PhpArrayAdapter) doesn't support updates without leading to potentially significant reserved yet unused memory.

@ossinkine
Copy link
Contributor Author

Metadata caching looks like the case when we can use PhpArrayAdapter

@ossinkine
Copy link
Contributor Author

So what about my proposal? I have an example implementation, do I need to create a PR? Or we don't need this and close the issue?

@ostrolucky
Copy link
Member

What's your actual proposal? Cache via PhpArrayAdapter by default, or what? Because that's not very useful when flex recipe overrides that, such change should go there instead.

@ossinkine
Copy link
Contributor Author

I propose to cache via PhpArrayAdapter always in production environment (actually when debug mode is disabled) and configured cache adapter is used as fallback in PhpArrayAdapter, exactly the same way as it used in symfony validator and serializer components.

@ostrolucky
Copy link
Member

The way I see it, this proposal would be accepted, but 2 conditions need to be met:

  • Similarly as we have doctrine.orm.metadata_cache_driver configuration, Symfony Validator also used to have framework.validation.cache configuration, which was properly deprecated and removed. I don't see keeping it and ignoring it in production expected behaviour, so I'm up for this proposal, but option should be deprecated.
  • As @nicolas-grekas mentioned, this makes sense only if we make sure this cache is warmed up. Please make sure that's the case currently, so we don't have high ratio of cache misses.

@ossinkine
Copy link
Contributor Author

@ostrolucky I couldn't find any description for branches so I can't decide which branch create PR to?

@ostrolucky
Copy link
Member

master, please

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

Successfully merging a pull request may close this issue.

4 participants