Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PropertyInfo] Use a single cache item per method#30523
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
9700ee0 tofa65dfaComparedunglas commentedMar 11, 2019
The patch looks good to me. |
dunglas commentedMar 11, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
However, it should target master, we don't consider performance fixes as bug fixes. |
deviantintegral commentedMar 11, 2019
One case we found is that if a new webserver is added to a pool due to autoscaling, having memcache + APC is useful in reducing first-load times. Thanks for the review. I will open a new PR so this is available in case anyone in the future wants to patch this in. |
…ntintegral)This PR was merged into the 4.3-dev branch.Discussion----------[PropertyInfo] Use a single cache item per method| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | #29977| License | MIT| Doc PR | noneReplacessymfony/symfony#30523 with a rebase to master.This PR changes how property metadata is cached, significantly reducing the number of calls made between PHP and the backend cache. Instead of storing one cache item per method and set of arguments, a single cache item is stored per method. This matches well with real-world use, where most properties in an object will need to be inspected.Note that the absolute numbers in the above PR are best case. In production environments where memcache is on a remote server, we were seeing multiple seconds consumed by memcache calls.Commits-------2a4f8a11d4 [PropertyInfo] Use a single cache item per method
…ntintegral)This PR was merged into the 4.3-dev branch.Discussion----------[PropertyInfo] Use a single cache item per method| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#29977| License | MIT| Doc PR | noneReplaces#30523 with a rebase to master.This PR changes how property metadata is cached, significantly reducing the number of calls made between PHP and the backend cache. Instead of storing one cache item per method and set of arguments, a single cache item is stored per method. This matches well with real-world use, where most properties in an object will need to be inspected.Note that the absolute numbers in the above PR are best case. In production environments where memcache is on a remote server, we were seeing multiple seconds consumed by memcache calls.Commits-------2a4f8a1 [PropertyInfo] Use a single cache item per method
This PR changes how property metadata is cached, significantly reducing the number of calls made between PHP and the backend cache. Instead of storing one cache item per method and set of arguments, a single cache item is stored per method. This matches well with real-world use, where most properties in an object will need to be inspected.
It's not clear to me if performance improvements fall under "bug fix", though I will say that we would have had to rewrite our code to remove PropertyInfo without this change.
Note that the absolute numbers in the above PR are best case. In production environments where memcache is on a remote server, we were seeing multiple seconds consumed by memcache calls.