Skip to content

Cache refactoring for performance improvements#491

Closed
mnapoli wants to merge 8 commits intomasterfrom
cache-refactoring
Closed

Cache refactoring for performance improvements#491
mnapoli wants to merge 8 commits intomasterfrom
cache-refactoring

Conversation

@mnapoli
Copy link
Copy Markdown
Member

@mnapoli mnapoli commented May 24, 2017

🎉 yuuuuge performance improvements incoming :)

Fixes #477 This pull request refactors the whole cache system.

Changes

Before:

  • every (cacheable) definition is cached as a separate entry
  • each definition is get on demand
  • each definition is set when not found
  • all PSR-16 caches are supported
  • APCu is recommended because given the number of get/set to the cache, all other caches are too slow

After:

  • only APCu is supported: that allows a much more optimized code, less errors from users, and a more "plug and play" solution (just enable the cache, nothing to configure)
  • all definitions are cached under a single cache entry
  • all the definitions are get at once: huge gain
  • each definition is set when not found (not optimisation here)

Things to look out for: cache stampede.

Performance comparison

Benchmark get-cache.php

Before:

  • no cache used: 224ms
  • APCu cache used: 181ms
  • gain thanks to the cache: 19%
  • profiling

After:

  • no cache used: 225ms
  • APCu cache used: 146ms
  • gain thanks to the cache: 35%
  • profiling

When you use the cache there is a 19% improvement (181 -> 146ms). Profiling

TODO

  • POC
  • optimize APCu calls to avoid many set in the same Container::get() call Not interesting
  • test with 3rd party DI container benchmarks
  • update the ContainerBuilder
  • configurable cache key prefix
  • the cache key should contain a hash of all sources? auto-refresh?
  • decide what to do with ArrayCache
  • update documentation
  • changelog

@mnapoli mnapoli added this to the 6.0 milestone May 24, 2017
@mnapoli mnapoli self-assigned this May 24, 2017
@mnapoli mnapoli force-pushed the cache-refactoring branch 2 times, most recently from ef0de28 to fc4db74 Compare May 24, 2017 17:27
@mnapoli mnapoli force-pushed the cache-refactoring branch from fc4db74 to f78897f Compare May 24, 2017 17:37
@mnapoli
Copy link
Copy Markdown
Member Author

mnapoli commented May 24, 2017

That is not really reassuring though: https://twitter.com/Ocramius/status/867477833123328000

Yes, but then I would never use APC/APCu for anything under high write load for the same reason

https://twitter.com/Ocramius/status/867477517623599105

Usually just the write being fast. APCu often gets cache corruption when under heavy write load, so we're under the same league

@mnapoli
Copy link
Copy Markdown
Member Author

mnapoli commented Jun 4, 2017

Made obsolete by #494

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor the cache to cache all definitions at once?

1 participant