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

Restore a definition cache for autowiring and Container::make() performances #564

Merged
merged 7 commits into from Jan 13, 2018

Conversation

Projects
None yet
3 participants
@mnapoli
Member

mnapoli commented Jan 6, 2018

This PR restores some kind of cache like there was in PHP-DI 4 and 5 (which was removed during v6's development). However this is not a full restore, this new cache is very simple and targeted at specific use cases.

Fixes #543 (indirectly) and related to #547 (workaround).

Compiling the container is the most efficient solution, but it has some limits. The following cases are not optimized:

  • autowired (or annotated) classes that are not declared in the configuration
  • wildcard definitions
  • usage of Container::make() or Container::injectOn() (because those are not using the compiled code)

If you make heavy use of those features, and if it slows down your application you can enable the caching system. The cache will ensure annotations or the reflection is not read again on every request.

The cache relies on APCu directly because it is the only cache system that makes sense (very fast to write and read). Other caches are not good options, this is why PHP-DI does not use PSR-6 or PSR-16.

To enable the cache:

$containerBuilder = new \DI\ContainerBuilder();
if (/* is production */) {
    $containerBuilder->enableDefinitionCache();
}

To be clear: compilation should be used first. This cache is only for specific use cases.

TODO:

  • Test that compiled entries are not fetched and stored into cache with get()
  • Store each definition in a separate cache entry
Restore a definition cache for autowiring and Container::make() perfo…
…rmances

This restores some kind of cache like there was in PHP-DI 4 and 5 (which was removed during v6's development). However this is not a full restore, this new cache is very simple and targeted at specific use cases.

Related to #543 and #547.

Compiling the container is the most efficient solution, but it has some limits. The following cases are not optimized:

- autowired (or annotated) classes that are not declared in the configuration
- wildcard definitions
- usage of `Container::make()` or `Container::injectOn()` (because those are not using the compiled code)

If you make heavy use of those features, and if it slows down your application you can enable the caching system. The cache will ensure annotations or the reflection is not read again on every request.

The cache relies on APCu directly because it is the only cache system that makes sense (very fast to write and read). Other caches are not good options, this is why PHP-DI does not use PSR-6 or PSR-16.

To enable the cache:

```php
$containerBuilder = new \DI\ContainerBuilder();
if (/* is production */) {
    $containerBuilder->enableDefinitionCache();
}
```

To be clear: compilation should be used first. This cache is only for specific use cases.
@amakhrov

@mnapoli

  1. I noticed that the new cache implementation is different from the one in v5: in v5 each definition is cached and fetched separately, while here it's all stored under a single cache key. Furthermore, every time a new definition is added to the cache, the whole cache content (which might be large) is sent back to APCu. I don't have enough knowledge to tell how well APCu handles this usage pattern. Any insights on that?
  2. If caching is enabled together with compiled container, APCu cache is applied for all object definitions, even those that have been compiled. It actually sounds like there is no point in using both caching and compilation for the same container. Is that correct?
// Object definitions are used with `make()`
|| ($definition instanceof ObjectDefinition)
// Autowired definitions cannot be all compiled and are used with `make()`
|| ($definition instanceof AutowireDefinition);

This comment has been minimized.

@amakhrov

amakhrov Jan 7, 2018

this seems redundant, as AutowireDefinition is an extension of ObjectDefinition (so already covered in like 86 above)

@amakhrov

amakhrov Jan 7, 2018

this seems redundant, as AutowireDefinition is an extension of ObjectDefinition (so already covered in like 86 above)

This comment has been minimized.

@mnapoli

mnapoli Jan 7, 2018

Member

Yes indeed, I wanted to make it extra clear (and to leave a comment that can be well understood), even though the second instruction will never be executed.

@mnapoli

mnapoli Jan 7, 2018

Member

Yes indeed, I wanted to make it extra clear (and to leave a comment that can be well understood), even though the second instruction will never be executed.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jan 7, 2018

Member

Thanks for the review @amakhrov.

You are right that this cache is pretty different, I forgot to mention I am reusing what I did in #491 (it was supposed to be an optimization of the cache before I scratched it completely after working on compilation).

The thing is I decided to reuse #491 when I first started to work on that and planned to cache everything. In the meantime, I realized I can skip caching most definitions except objects/autowiring. So in the end, there will be much less entries in the cache (especially with compilation. So I'm not sure storing everything in one entry is the best strategy anymore.

I don't have enough knowledge to tell how well APCu handles this usage pattern. Any insights on that?

Same here, so you might be right in that it's safer to keep v5's behavior.

Yes it will work as you describe, but:

  • if an entry is compiled and you get it, the definition will be skipped entirely so the cache will not change anything (I will add a test for that). However make does not use the compiled code (this is #543) so this PR will help those that make heavy use of make
  • if you don't define all your classes in the config and use autowiring, those entries will not be compiled (so the cache will help)
  • if you use wildcard definitions those will not be compiled too (this is #547)

So it sometimes make sense to enable both features.

Member

mnapoli commented Jan 7, 2018

Thanks for the review @amakhrov.

You are right that this cache is pretty different, I forgot to mention I am reusing what I did in #491 (it was supposed to be an optimization of the cache before I scratched it completely after working on compilation).

The thing is I decided to reuse #491 when I first started to work on that and planned to cache everything. In the meantime, I realized I can skip caching most definitions except objects/autowiring. So in the end, there will be much less entries in the cache (especially with compilation. So I'm not sure storing everything in one entry is the best strategy anymore.

I don't have enough knowledge to tell how well APCu handles this usage pattern. Any insights on that?

Same here, so you might be right in that it's safer to keep v5's behavior.

Yes it will work as you describe, but:

  • if an entry is compiled and you get it, the definition will be skipped entirely so the cache will not change anything (I will add a test for that). However make does not use the compiled code (this is #543) so this PR will help those that make heavy use of make
  • if you don't define all your classes in the config and use autowiring, those entries will not be compiled (so the cache will help)
  • if you use wildcard definitions those will not be compiled too (this is #547)

So it sometimes make sense to enable both features.

@amakhrov

This comment has been minimized.

Show comment
Hide comment
@amakhrov

amakhrov Jan 9, 2018

@mnapoli I tested performance in multiple scenarios.

Results:

# PHP-DI Version Test Case dispatch() time, ms DI\injectOn() time, ms %
1 5 ArrayCache 1293 155 10
2 5 APCu - first call 1324 167 11.2
3 5 APCu - second call 1199 35 2.6
4 6-master Unoptimized 1325 167 11.2
5 6-master Compiled Container 1297 155 10.7
6 6-apcu Compiled + Cache - first call 1483 186 11
7 6-apcu Compiled + Cache - second call 1410 36 2.2

Test methodology:

  • All tests were done in PHP 7.1 on Mac, with XDebug profiling enabled.
  • I ran every test case twice - the results are pretty much the same.
  • For the Compiled tests I added definitions (MyController1::class => autowire()) for a couple of top-level controllers to the php-di config file.
  • Also the CompiledContainer.php file was generated in advance before running the actual tests.
  • Absolute timing doesn't matter much here, since it's all delayed by the profiler. What matters is how the results compare to each other.
  • The particular program execution path I was testing doesn't have any make() calls.

Highlights:

  • Caching in v5 and v6 has the same performance both for the cold cache (2, 6) and for the warm cache (3 and 7).
  • Compiled container in v6 doesn't make a significant performance difference. Looking at the generated container code, I can see if only contains definition for top-level classes (auto-wired). However, each all its dependencies are delegated to a regular resolving mechanism. Since in my case most of the underlying classes are also auto-wired, resolving seems to take a lot of time to parse annotations and typehints. This was kind of surprising to me: I expected the whole dependencies tree to be compiled.

amakhrov commented Jan 9, 2018

@mnapoli I tested performance in multiple scenarios.

Results:

# PHP-DI Version Test Case dispatch() time, ms DI\injectOn() time, ms %
1 5 ArrayCache 1293 155 10
2 5 APCu - first call 1324 167 11.2
3 5 APCu - second call 1199 35 2.6
4 6-master Unoptimized 1325 167 11.2
5 6-master Compiled Container 1297 155 10.7
6 6-apcu Compiled + Cache - first call 1483 186 11
7 6-apcu Compiled + Cache - second call 1410 36 2.2

Test methodology:

  • All tests were done in PHP 7.1 on Mac, with XDebug profiling enabled.
  • I ran every test case twice - the results are pretty much the same.
  • For the Compiled tests I added definitions (MyController1::class => autowire()) for a couple of top-level controllers to the php-di config file.
  • Also the CompiledContainer.php file was generated in advance before running the actual tests.
  • Absolute timing doesn't matter much here, since it's all delayed by the profiler. What matters is how the results compare to each other.
  • The particular program execution path I was testing doesn't have any make() calls.

Highlights:

  • Caching in v5 and v6 has the same performance both for the cold cache (2, 6) and for the warm cache (3 and 7).
  • Compiled container in v6 doesn't make a significant performance difference. Looking at the generated container code, I can see if only contains definition for top-level classes (auto-wired). However, each all its dependencies are delegated to a regular resolving mechanism. Since in my case most of the underlying classes are also auto-wired, resolving seems to take a lot of time to parse annotations and typehints. This was kind of surprising to me: I expected the whole dependencies tree to be compiled.
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jan 9, 2018

Member

Caching in v5 and v6 has the same performance both for the cold cache (2, 6) and for the warm cache (3 and 7).

This is great news, that means that with the PR there should be at worst no performance regression in v6 (and at best of course the compiled container should speed things up).

I expected the whole dependencies tree to be compiled.

😮 I will not lie, I simply did not think about that! But absolutely, this makes a lot of sense! It would also solve partially #547

I've opened #565 to keep track of this. It seems it might be easy to implement, however I don't want to delay 6.0 any longer than necessary so this kind of improvements could come in a 6.1.

Member

mnapoli commented Jan 9, 2018

Caching in v5 and v6 has the same performance both for the cold cache (2, 6) and for the warm cache (3 and 7).

This is great news, that means that with the PR there should be at worst no performance regression in v6 (and at best of course the compiled container should speed things up).

I expected the whole dependencies tree to be compiled.

😮 I will not lie, I simply did not think about that! But absolutely, this makes a lot of sense! It would also solve partially #547

I've opened #565 to keep track of this. It seems it might be easy to implement, however I don't want to delay 6.0 any longer than necessary so this kind of improvements could come in a 6.1.

@mnapoli mnapoli referenced this pull request Jan 9, 2018

Merged

Compile recursively all references found during compilation #566

2 of 2 tasks complete
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jan 9, 2018

Member

@amakhrov actually it was even simpler than I thought, I created a WIP here (no tests yet): #566

Could you try this branch out? Thanks!

Member

mnapoli commented Jan 9, 2018

@amakhrov actually it was even simpler than I thought, I created a WIP here (no tests yet): #566

Could you try this branch out? Thanks!

@amakhrov

This comment has been minimized.

Show comment
Hide comment
@amakhrov

amakhrov Jan 9, 2018

@mnapoli I'll check that later today.

As for the caching performance in this PR: It just stroke me that I only tested a single request scenario.
In a production there will be multiple simultaneous requests. I'm concerned that with the new cache implementation those simultaneous php processes will overwrite cache after each other - since they all use the same cache key.
I guess eventually this will stabilize, but it can take a few iterations, depending on the parallelism pattern.
I don't know to properly profile such scenarios, though. So I can only theoretically reason about that at the moment.

amakhrov commented Jan 9, 2018

@mnapoli I'll check that later today.

As for the caching performance in this PR: It just stroke me that I only tested a single request scenario.
In a production there will be multiple simultaneous requests. I'm concerned that with the new cache implementation those simultaneous php processes will overwrite cache after each other - since they all use the same cache key.
I guess eventually this will stabilize, but it can take a few iterations, depending on the parallelism pattern.
I don't know to properly profile such scenarios, though. So I can only theoretically reason about that at the moment.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jan 9, 2018

Member

You could run apache bench with concurrency (ab -c 10) to simulate that. However I will remove that behavior and store each entry in a separate key.

Member

mnapoli commented Jan 9, 2018

You could run apache bench with concurrency (ab -c 10) to simulate that. However I will remove that behavior and store each entry in a separate key.

mnapoli added some commits Jan 9, 2018

Store each definition in a separate cache entry
Since the container is compiled we will store much less entries (definitions) in the cache. So it makes sense to separate each entry because there should not be a lot of entries. That will prevent cache stamped.
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jan 9, 2018

Member

Each definition is now stored in a separate cache entry.

Member

mnapoli commented Jan 9, 2018

Each definition is now stored in a separate cache entry.

public function getDefinition(string $name)
{
$definition = apcu_fetch(self::CACHE_KEY . $name);

This comment has been minimized.

@hectorj

hectorj Jan 11, 2018

I think using apcu_entry could reduce the risk of cache stampede. What do you think?

@hectorj

hectorj Jan 11, 2018

I think using apcu_entry could reduce the risk of cache stampede. What do you think?

This comment has been minimized.

@mnapoli

mnapoli Jan 11, 2018

Member

👍 I initially went with apcu_fetch because I was storing all definitions in the same key but now that all are stored in separate entries it would make more sense. I'll change that.

@mnapoli

mnapoli Jan 11, 2018

Member

👍 I initially went with apcu_fetch because I was storing all definitions in the same key but now that all are stored in separate entries it would make more sense. I'll change that.

This comment has been minimized.

@mnapoli

mnapoli Jan 13, 2018

Member

Just remembered why we can't use apcu_entry -> some entries cannot be cached (objects, etc.)

@mnapoli

mnapoli Jan 13, 2018

Member

Just remembered why we can't use apcu_entry -> some entries cannot be cached (objects, etc.)

@mnapoli mnapoli added this to the 6.0 milestone Jan 13, 2018

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jan 13, 2018

Member

Given that the performance regression is resolved with this I am going to merge it and continue improving the performances of the compiled container in #566.

Thank you!

Member

mnapoli commented Jan 13, 2018

Given that the performance regression is resolved with this I am going to merge it and continue improving the performances of the compiled container in #566.

Thank you!

@mnapoli mnapoli merged commit 360bf15 into master Jan 13, 2018

5 checks passed

Scrutinizer 10 updated code elements
Details
continuous-integration/prettyci The PrettyCI build has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 93.987%
Details

@mnapoli mnapoli deleted the apcu-cache branch Jan 13, 2018

@juliangut juliangut referenced this pull request Jan 15, 2018

Closed

enable definition cache #2

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