Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Added support for timings in ArgumentValueResolvers#26833
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
| */ | ||
| publicfunctionsupports(Request$request,ArgumentMetadata$argument):bool | ||
| { | ||
| $method =\get_class($this->inner).'::'.__FUNCTION__; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could also just do$method = \get_class($this->inner).'::supports'; and resolve for the other. Not sure what is preferred. Opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍for__FUNCTION__
fabpot commentedApr 6, 2018
4.1 is still open for small additions. |
linaori commentedApr 6, 2018
I've updated the changelogs to be for 4.1 |
UPGRADE-4.1.md Outdated
| HttpKernel | ||
| ---------- | ||
| * Added the `Symfony\Component\HttpKernel\Controller\ArgumentResolver\TraceableValueResolver` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is no upgrade path here 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's what I meant actually. There is no BC break to document here, no upgrade path. We don't mention features inUPGRADE files 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done 😄
| */ | ||
| publicfunctionsupports(Request$request,ArgumentMetadata$argument):bool | ||
| { | ||
| $method =\get_class($this->inner).'::'.__FUNCTION__; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍for__FUNCTION__
| $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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
'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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we are used todebug.$id
| foreach ($container->findTaggedServiceIds('controller.argument_value_resolver')as$id =>$tags) { | ||
| $container->register($id.'.traceable', TraceableValueResolver::class) | ||
| ->setDecoratedService($id) | ||
| ->setArguments(array(newReference($id.'.traceable.inner'),newReference('debug.stopwatch'))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this binds the feature to FrameworkBundle as it's the one registeringdebug.stopwatch. I would make itnull if invalid and instantiatesStopWatch in the TraceableValueResolver constructor if not injected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| return; | ||
| } | ||
| $resolvers =$this->findAndSortTaggedServices($this->argumentValueResolverTag,$container); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
assignment does not seem necessary, leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's actually being used fornew 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oops, missed the loop!
| $id = (string)$resolverReference; | ||
| $container->register('debug.'.$id, TraceableValueResolver::class) | ||
| ->setDecoratedService($id) | ||
| ->setArguments(array(newReference('debug.'.$id),newReference('debug.stopwatch', ContainerInterface::NULL_ON_INVALID_REFERENCE))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be"debug.$id.inner"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Woops, I think I've removed too much 😅
linaori commentedApr 18, 2018
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 left a comment• 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 Could have a line in the http-kernel changelog
linaori commentedApr 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? |
chalasr commentedApr 18, 2018
@iltar I mean only the CHANGELOG file, not upgrade ones :) |
linaori commentedApr 18, 2018
Something in the form of: |
chalasr commentedApr 18, 2018
👌 for me |
fabpot commentedApr 20, 2018
Thank you@iltar. |
…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
…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 <!-- seehttps://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/AI 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
…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 <!-- seehttps://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
…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 <!-- seehttps://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.Sincesymfony/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
Uh oh!
There was an error while loading.Please reload this page.
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