Extract IriConverter::getRouteName into RouteNameResolverInterface #721
Conversation
teohhanhui
force-pushed
the
iri-converter-perf
branch
4 times, most recently
from
aa1c689
to
9b1840a
Compare
|
Would it be possible to have a blackfire profiling on the demo app with it? |
|
Does Blackfire offer anything for open source projects? The free plan only has 1-day data retention... |
| $routeName = $this->decorated->getRouteName($resourceClass, $collection); | ||
|
|
||
| if (isset($cacheItem)) { | ||
| try { |
There was a problem hiding this comment.
what about an early return here to avoid having the try/catch nested?
Oh, I wasn't aware they changed that. Even if it's one day it's fine, we need it only for a quick review otherwise a screenshot of the result is enough. |
| <argument type="service" id="api_platform.router" /> | ||
| </service> | ||
|
|
||
| <service id="api_platform.route_name_resolver.cached" class="ApiPlatform\Core\Bridge\Symfony\Routing\CachedRouteNameResolver" decorates="api_platform.route_name_resolver" decoration-priority="-10" public="false"> |
There was a problem hiding this comment.
Why is the decoration-priority attribute necessary?
There was a problem hiding this comment.
To convey the intention that this should run dead last, and so that any user-declared decorators would be correctly wrapped by default (decoration priority 0). It's good to have, and consistent with the decorated metadata factory services...
|
Profiling of GET
|
|
In our project for example, there are 409 calls to Before (warm): 320ms (28%) |
| @@ -20,7 +20,7 @@ | |||
| use ApiPlatform\Core\Util\ClassInfoTrait; | |||
| use Symfony\Component\PropertyAccess\PropertyAccess; | |||
| use Symfony\Component\PropertyAccess\PropertyAccessorInterface; | |||
| use Symfony\Component\Routing\Exception\ExceptionInterface; | |||
| use Symfony\Component\Routing\Exception\ExceptionInterface as RoutingExceptionInterface; | |||
There was a problem hiding this comment.
Because ExceptionInterface is not specific enough, especially considering we also have our own ExceptionInterface.
There was a problem hiding this comment.
SymfonyRoutingExceptionInterface then?
There was a problem hiding this comment.
@theofidry That seems like overkill? 😛 Let's try to strike a balance...
|
This is great ! @teohhanhui 👍 |
teohhanhui
force-pushed
the
iri-converter-perf
branch
from
9b1840a
to
23b0f83
Compare
|
Comments addressed. |
| $cacheItemPoolProphecy->save($cacheItemProphecy)->willReturn(true)->shouldBeCalled(); | ||
|
|
||
| $decoratedProphecy = $this->prophesize(RouteNameResolverInterface::class); | ||
| $decoratedProphecy->getRouteName('AppBundle\Entity\User', true)->willReturn('some_collection_route')->shouldBeCalled(); |
| { | ||
| $routeCollection = new RouteCollection(); | ||
| $routeCollection->add('some_collection_route', new Route('/some/collection/path', [ | ||
| '_api_resource_class' => 'AppBundle\Entity\User', |
There was a problem hiding this comment.
The ::class constant can be used.
|
What do you think about removing the |
I don't think we should remove the rule. It can still help us catch some actual code duplication. No harm, right? |
|
Thanks! |
Extract IriConverter::getRouteName into RouteNameResolverInterface
This allows caching of route name resolving, which should result in a significant performance gain (please verify).