[DI] Only rebuild autowiring cache when actually needed #18144
Conversation
d40c146 to
8201a57
Compare
8201a57 to
df536ca
Compare
|
I had an idea: we could make the new resource checker "generic" - something that accepts any class-related information (method + arg info, annotation info, etc). Then it could be used by autowiring, but also by the |
ea23c0e to
4b521c5
Compare
|
This is idea is now working, and it's quite simple and self-contained :). /cc @dunglas Status: Needs Review |
| $constructorParams = $constructor ? $constructor->getParameters() : array(); | ||
| $constructorArgumentsForResource = array(); | ||
| foreach ($constructorParams as $parameter) { | ||
| $constructorArgumentsForResource[] = $parameter->getClass(); |
There was a problem hiding this comment.
you should store class names here rather than ReflectionClass instance.
There was a problem hiding this comment.
and this misses several cases which impact the autowiring:
- default options for args
- classes which don't exist (but autowiring working because the typehint allows injecting
null). This would trigger a ReflectionException in your code here, thus breaking autowiring - setters (as autowiring supports them now IIRC)
|
status: needs work |
…n it's *actually* needed
e8598ee to
d0b600a
Compare
|
@stof yes, thank you so much! Those were really great cases I hadn't thought of before! I refactored to store a string representation of the arguments in the resource, which pretty much catches all the cases in a very simple way. It may over-match on some things, like changing an argument's name, but not its type-hint - but I think that's a micro-optimization (for the dev env) that would be much more complex to implement. The one case that's still not covered - and never was with the old method - is a non-existent class type hint suddenly becoming existent (i.e. a new lib was installed). But, in practice, this is quite rare and installing new packages cause a cache refresh. Also, that is the current behavior anyways. Status: Needs Review |
| // of determining valid "setter" methods | ||
| foreach ($reflectionClass->getMethods(\ReflectionMethod::IS_PUBLIC) as $reflectionMethod) { | ||
| $name = $reflectionMethod->getName(); | ||
| if (isset($methodsCalled[$name]) || $reflectionMethod->isStatic() || 1 !== $reflectionMethod->getNumberOfParameters() || 0 !== strpos($name, 'set')) { |
There was a problem hiding this comment.
isset($methodsCalled[$name]) will never be set, as $methodsCalled is undefined
|
|
||
| public function testIsFreshSameConstructorArgs() | ||
| { | ||
| $oldResource = AutowirePass::createResourceForClass( |
There was a problem hiding this comment.
|
Comments handled! In the last commit, I tried to get more fine-grained on the argument info we need. I had this at one point earlier, but decided to use the string-casting method because it makes the code simpler and potentially less buggy (i.e. is there some edge-case that this is missing?). The string casting is overly-eager, but that means it errs on the side of clearing the cache too often, which is not a big deal. If you guys do like the simple method better, we can revert my most recent commit and go back. That would still be way better than the current situation :). Thanks! |
|
Thank you @weaverryan. |
| @@ -278,4 +310,25 @@ private function addServiceToAmbiguousType($id, $type) | |||
| } | |||
| $this->ambiguousServiceTypes[$type][] = $id; | |||
| } | |||
|
|
|||
| static private function getResourceMetadataForMethod(\ReflectionMethod $method) | |||
|
@weaverryan or anyone else, could you please have a look at the current test failures? They are related to this PR. |
| { | ||
| $this->assertTrue($this->resource->isFresh($this->time), '->isFresh() returns true if the resource has not changed in same second'); | ||
| $this->assertTrue($this->resource->isFresh($this->time + 10), '->isFresh() returns true if the resource has not changed'); | ||
| $this->assertFalse($this->resource->isFresh($this->time - 86400), '->isFresh() returns false if the resource has been updated'); |
There was a problem hiding this comment.
Ok it this line, the object is returned instead of false.
This PR was squashed before being merged into the 3.1-dev branch (closes #18422). Discussion ---------- [DependencyInjection] Fix tests of #18144 | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ Commits ------- 5e7dbae [DependencyInjection] Fix tests of #18144
|
Ah, sorry about that @nicolas-grekas! I made a mistake on my final commits! The linked PR did not fix the issue - but I added a comment here: https://github.com/symfony/symfony/pull/18422/files#r58362092. I'll make sure that either @HeahDude will fix it (if he's nice enough and has time), otherwise I'll add a PR to fix it :) |
This PR was merged into the 3.1-dev branch. Discussion ---------- fix Autowiring tests of #18144 | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | Tests pass? | yes | Fixed issue | #18422 (comment) | License | MIT Commits ------- 0cbf04a [DI] fix Autowiring tests of #18144
With autowiring, the container is currently rebuilt when an autowired class is modified in any way. That makes it unusable for large systems. However, this doesn't need to be true: we only need to rebuild the cache if a construct argument changes. This adds that change, and it's quite simple
This idea can/should also be adapted easily to the
@Routecache (I don't believe it has something "smart" like this).Thanks!