[HttpKernel] Added support for timings in ArgumentValueResolvers #26833
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
linaori
commented
Apr 6, 2018
| */ | ||
| public function supports(Request $request, ArgumentMetadata $argument): bool | ||
| { | ||
| $method = \get_class($this->inner).'::'.__FUNCTION__; |
There was a problem hiding this comment.
Could also just do $method = \get_class($this->inner).'::supports'; and resolve for the other. Not sure what is preferred. Opinions?
|
4.1 is still open for small additions. |
|
I've updated the changelogs to be for 4.1 |
ogizanagi
reviewed
Apr 6, 2018
UPGRADE-4.1.md
Outdated
| HttpKernel | ||
| ---------- | ||
|
|
||
| * Added the `Symfony\Component\HttpKernel\Controller\ArgumentResolver\TraceableValueResolver` |
There was a problem hiding this comment.
There is no upgrade path here 😄
There was a problem hiding this comment.
Should I remove it in that case? It doesn't cause harm to add it, but it's not really of any value to developers as it's primarily internal.
There was a problem hiding this comment.
That's what I meant actually. There is no BC break to document here, no upgrade path. We don't mention features in UPGRADE files 😄
| */ | ||
| public function supports(Request $request, ArgumentMetadata $argument): bool | ||
| { | ||
| $method = \get_class($this->inner).'::'.__FUNCTION__; |
chalasr
reviewed
Apr 17, 2018
| $resolvers = $this->findAndSortTaggedServices($this->argumentValueResolverTag, $container); | ||
|
|
||
| if ($container->getParameter('kernel.debug') && class_exists(Stopwatch::class)) { | ||
| foreach ($container->findTaggedServiceIds('controller.argument_value_resolver') as $id => $tags) { |
There was a problem hiding this comment.
'controller.argument_value_resolver' => $this->argumentValueResolverTag
|
|
||
| if ($container->getParameter('kernel.debug') && class_exists(Stopwatch::class)) { | ||
| foreach ($container->findTaggedServiceIds('controller.argument_value_resolver') as $id => $tags) { | ||
| $container->register($id.'.traceable', TraceableValueResolver::class) |
| foreach ($container->findTaggedServiceIds('controller.argument_value_resolver') as $id => $tags) { | ||
| $container->register($id.'.traceable', TraceableValueResolver::class) | ||
| ->setDecoratedService($id) | ||
| ->setArguments(array(new Reference($id.'.traceable.inner'), new Reference('debug.stopwatch'))); |
There was a problem hiding this comment.
this binds the feature to FrameworkBundle as it's the one registering debug.stopwatch. I would make it null if invalid and instantiates StopWatch in the TraceableValueResolver constructor if not injected
There was a problem hiding this comment.
The class is kinda useless if null is inserted though 😅 there's no way of fetching the stopwatch. The only thing that happens, is that it doesn't crash on NULL.
| @@ -40,9 +43,19 @@ public function process(ContainerBuilder $container) | |||
| return; | |||
| } | |||
|
|
|||
| $resolvers = $this->findAndSortTaggedServices($this->argumentValueResolverTag, $container); | |||
There was a problem hiding this comment.
assignment does not seem necessary, leftover?
There was a problem hiding this comment.
It's actually being used for new IteratorArgument($resolvers). I initially wasn't able to use this as I needed the id, but casting the reference to a string gives me exactly that, saving a call to the container.
chalasr
reviewed
Apr 17, 2018
| $id = (string) $resolverReference; | ||
| $container->register('debug.'.$id, TraceableValueResolver::class) | ||
| ->setDecoratedService($id) | ||
| ->setArguments(array(new Reference('debug.'.$id), new Reference('debug.stopwatch', ContainerInterface::NULL_ON_INVALID_REFERENCE))); |
There was a problem hiding this comment.
Woops, I think I've removed too much 😅
|
Service definitions should be fixed now, tests are passing 👍 My PC only froze for 10 minutes after which I rebooted via the on/off button, due to a minor recursion issue I created during the process. |
chalasr
approved these changes
Apr 18, 2018
|
@chalasr I removed the line where I mentioned adding it based on feedback earlier. I can put something back in there if desired. What were you thinking of? |
|
@iltar I mean only the CHANGELOG file, not upgrade ones :) |
|
Something in the form of: |
|
👌 for me |
fabpot
approved these changes
Apr 20, 2018
|
Thank you @iltar. |
fabpot
added a commit
that referenced
this pull request
Apr 20, 2018
…eResolvers (iltar) This PR was squashed before being merged into the 4.1-dev branch (closes #26833). Discussion ---------- [HttpKernel] Added support for timings in ArgumentValueResolvers | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ This feature provides timings for the supports and resolve methods for each individual argument value resolver. It was already possible to see the `controller.get_arguments` timing, but it was impossible to track a possible performance issue any further without using different tools. I've explicitly added the `supports` method as well, as it might otherwise hide performance issues. The `ServiceValueResolver` for example, does a `container::has`, which in turn might trigger a factory method, which might trigger a query for example. ~~Due to the feature freeze I've already added this to 4.2. If for some reason it's okay to still merge it into 4.1, I can update the upgrade files.~~ - Changed to 4.1 ##### *Without performance issue*  ##### *With performance issue*  Commits ------- 1c0d892 [HttpKernel] Added support for timings in ArgumentValueResolvers
Merged
This was referenced May 9, 2018
fabpot
added a commit
that referenced
this pull request
May 14, 2018
…atory (ogizanagi) This PR was merged into the 4.1 branch. Discussion ---------- [HttpKernel] Make TraceableValueResolver $stopwatch mandatory | Q | A | ------------- | --- | Branch? | 4.1 <!-- see below --> | Bug fix? | no | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | #26833 (comment) <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | N/A I understand why this was suggested in #26833 (comment), but as stated by @iltar , I don't think it makes sense to register a traceable resolver instantiating a Stopwatch itself, as there is no way to fetch it and wouldn't be a shared instance, probably defeating the feature and registering a useless decorator. Instead, let's make the stopwatch mandatory and make the service id to use in the pass configurable. Commits ------- 585ae7c [HttpKernel] Make TraceableValueResolver $stopwatch mandatory
fabpot
added a commit
that referenced
this pull request
May 14, 2018
…voke controllers method (ogizanagi) This PR was merged into the 4.1 branch. Discussion ---------- [HttpKernel] Fix services are no longer injected into __invoke controllers method | Q | A | ------------- | --- | Branch? | 4.1 <!-- see below --> | Bug fix? | yes | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | #27208 <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | N/A _TL;DR:_ The `RemoveEmptyControllerArgumentLocatorsPass` is the one adding the `Controller::_invoke` => `Controller` shortcut missing from the service locator. It isn't properly executed on some cases. This fixes it. Since #26833, the resolvers are decorated by a `TraceableValueResolver`, which usually isn't much an issue to deal within passes. But the `RemoveEmptyControllerArgumentLocatorsPass` happens late (`TYPE_BEFORE_REMOVING`), when decoration inheritance is already resolved, so accessing `$controllerLocator = $container->getDefinition((string) $serviceResolver->getArgument(0));` isn't accessing the controller locator, but the decorated service instead. Commits ------- ee44903 [HttpKernel] Fix services are no longer injected into __invoke controllers method
symfony-splitter
pushed a commit
to symfony/http-kernel
that referenced
this pull request
May 14, 2018
…voke controllers method (ogizanagi) This PR was merged into the 4.1 branch. Discussion ---------- [HttpKernel] Fix services are no longer injected into __invoke controllers method | Q | A | ------------- | --- | Branch? | 4.1 <!-- see below --> | Bug fix? | yes | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | #27208 <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | N/A _TL;DR:_ The `RemoveEmptyControllerArgumentLocatorsPass` is the one adding the `Controller::_invoke` => `Controller` shortcut missing from the service locator. It isn't properly executed on some cases. This fixes it. Since symfony/symfony#26833, the resolvers are decorated by a `TraceableValueResolver`, which usually isn't much an issue to deal within passes. But the `RemoveEmptyControllerArgumentLocatorsPass` happens late (`TYPE_BEFORE_REMOVING`), when decoration inheritance is already resolved, so accessing `$controllerLocator = $container->getDefinition((string) $serviceResolver->getArgument(0));` isn't accessing the controller locator, but the decorated service instead. Commits ------- ee44903 [HttpKernel] Fix services are no longer injected into __invoke controllers method
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This feature provides timings for the supports and resolve methods for each individual argument value resolver. It was already possible to see the
controller.get_argumentstiming, but it was impossible to track a possible performance issue any further without using different tools. I've explicitly added thesupportsmethod as well, as it might otherwise hide performance issues. TheServiceValueResolverfor example, does acontainer::has, which in turn might trigger a factory method, which might trigger a query for example.Due to the feature freeze I've already added this to 4.2. If for some reason it's okay to still merge it into 4.1, I can update the upgrade files.- Changed to 4.1Without performance issue
With performance issue