Skip to content
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

Request controller argument is first resolved as Doctrine entity, causing 40-50ms performance loss #54337

Open
Xesau opened this issue Mar 19, 2024 · 7 comments

Comments

@Xesau
Copy link
Contributor

Xesau commented Mar 19, 2024

Symfony version(s) affected

7.0.5

Description

For the tag controller.argument_value_resolver, the argument_resolver.request service has a priority of 50, while doctrine.orm.entity_value_resolver has a priority of 110. As a result, EntityValueResolver from DoctrineBundle first tries to resolve the Symfony\Component\HttpFoundation\Request controller argument as an entity. This is nonsensical and will cause Doctrine to try to find an entity manager and repository for it. In my case this leads to an extra 40-50ms of Total execution time on average.
By changing the priority of argument_resolver.request to 150, the Request argument is resolved immediately without this detour.

How to reproduce

Create a clean installation of symfony/skeleton and require symfony/webapp-pack. Create a controller with a Request $request function argument and visit it. Inspect the Performance profiler page for the request.

Possible Solution

No response

Additional Context

No response

@Xesau Xesau added the Bug label Mar 19, 2024
@Xesau Xesau changed the title Request controller argument is first resolved as entity, causing 40-50ms performance loss Request controller argument is first resolved as Doctrine entity, causing 40-50ms performance loss Mar 19, 2024
@MatTheCat
Copy link
Contributor

You can use the ValueResolver attribute to specify which resolver must be called for a given parameter; see https://symfony.com/doc/current/controller/value_resolver.html#managing-value-resolvers.

@Xesau
Copy link
Contributor Author

Xesau commented Mar 19, 2024

@MatTheCat That is true, but that is not my point. The documentation (https://symfony.com/doc/current/controller.html, as well as the page you link) teaches us that we can obtain the Request object (through the request value resolver) by specifying just a Request argument in the controller function. In my opinion, the fact that resolution is attempted by the Doctrine entity resolver before the Request is unexpected behavior. One should not be required to specify what is assumed to be the default. The current configuration is therefore not sensible and should be changed. Or the documentation should explicity state that you should never rely on the type annotation alone and always specify which resolver should be used.

@MatTheCat
Copy link
Contributor

MatTheCat commented Mar 19, 2024

I was not making a point, just giving you a short-term solution.

EntityValueResolver’s priority is so high because it must be greater than RequestAttributeValueResolver’s, but this already triggered some other changes (like #48032).

I cannot think of use-cases where it would be problematic to increase the RequestValueResolver’s priority; will you open a PR?

@smnandre
Copy link
Member

There was one, not sure if that is still the case: doctrine/DoctrineBundle#1554 (comment)

@MatTheCat
Copy link
Contributor

@smnandre the comment you linked mentions the RequestAttributeValueResolver, not RequestValueResolver.

It actually explains why “EntityValueResolver’s priority […] must be greater than RequestAttributeValueResolver’s” 😁

@smnandre
Copy link
Member

#SimonPleaseTalkLessAndSleepMoreS07E07 .... 😶‍🌫️

@nicolas-grekas
Copy link
Member

40–50ms

That's a lot and that's surprising. Normally this should be fast because this should all be just about loading pregenerated code.
Can you please try to benchmark what takes so long? Maybe you're missing some metadata cache or something else?

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

Successfully merging a pull request may close this issue.

7 participants