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

[FrameworkBundle] Improve performance of ControllerNameParser #20374

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
10 participants
@enumag
Copy link
Contributor

commented Nov 1, 2016

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Today I was searching for bottlenecks in my application using Blackfire. And among other things I found one in Symfony. Blackfire showed that Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser::findAlternative() was called almost 300 times which took 28 miliseconds.

It turns out that Symfony\Bundle\FrameworkBundle\Routing\DelegatingLoader::load() is calling ControllerNameParser::parse() without actually needing to do so because $controller is in the class::method notation already. ControllerNameParser threw an exception, DelegatingLoader caught and ignored it - that's ok. The problem is that generating the exception message took a lot of time because findAlternative is slow. In my case it called the levenshtein function over 5000 times which was completely useless because the exception is ignored anyway.

// The second condition is for cases when this method is called with the
// class::method string instead of a:b:c. It's important to fail early in this
// case because calling findAlternative needlessly is bad for performance.
if (3 !== count($parts) || $parts[1] === '') {

This comment has been minimized.

Copy link
@stloyd

stloyd Nov 1, 2016

Contributor

Second check would be faster so IMO should be first ;)

This comment has been minimized.

Copy link
@enumag

enumag Nov 1, 2016

Author Contributor

Can't be first because $parts[1] might not exist.

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 1, 2016

Contributor

Perhaps check [0] and [2] as well for emptiness? As those are also not a valid "a:b:c" controller string.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Nov 2, 2016

Member

we prefer yoda style: '' === $parts[1]

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

Just wondering: would it make sense to add this check in DelegatingLoader instead?
Something like:

--- a/src/Symfony/Bundle/FrameworkBundle/Routing/DelegatingLoader.php
+++ b/src/Symfony/Bundle/FrameworkBundle/Routing/DelegatingLoader.php
@@ -21,7 +21,7 @@ use Psr\Log\LoggerInterface;
  * DelegatingLoader delegates route loading to other loaders using a loader resolver.
  *
  * This implementation resolves the _controller attribute from the short notation
- * to the fully-qualified form (from a:b:c to class:method).
+ * to the fully-qualified form (from a:b:c to class::method).
  *
  * @author Fabien Potencier <fabien@symfony.com>
  */
@@ -85,7 +85,8 @@ class DelegatingLoader extends BaseDelegatingLoader
         $this->loading = false;

         foreach ($collection->all() as $route) {
-            if ($controller = $route->getDefault('_controller')) {
+            $controller = $route->getDefault('_controller');
+            if ($controller && false === strpos($controller, '::')) {
                 try {
                     $controller = $this->parser->parse($controller);
                 } catch (\InvalidArgumentException $e) {
@enumag

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2016

@nicolas-grekas I can add that as well but the change in ControllerNameParser should be done regardless in my opinion.

@enumag enumag force-pushed the enumag:patch-18 branch from f9b9774 to ac284c3 Nov 5, 2016

@enumag

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2016

@nicolas-grekas @ro0NL All fixed.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Nov 5, 2016

👍

1 similar comment
@xabbuh

This comment has been minimized.

Copy link
Member

commented Nov 5, 2016

👍

@ro0NL
Copy link
Contributor

left a comment

👍

if (3 !== count($parts = explode(':', $controller))) {
$parts = explode(':', $controller);
// The second condition is for cases when this method is called with the

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 5, 2016

Contributor

I think the exception message is already straightforward now? Imo. no need for this comment.

@@ -85,7 +85,8 @@ public function load($resource, $type = null)
$this->loading = false;
foreach ($collection->all() as $route) {
if ($controller = $route->getDefault('_controller')) {
$controller = $route->getDefault('_controller');
if ($controller && false === strpos($controller, '::')) {

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 5, 2016

Contributor

Instead, maybe add a comment here about skipping FQCN's for performance.

@enumag enumag force-pushed the enumag:patch-18 branch from ac284c3 to 08cc54f Nov 6, 2016

@enumag

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2016

@ro0NL Like this?

@javiereguiluz javiereguiluz added this to the 3.2 milestone Nov 7, 2016

if ($controller = $route->getDefault('_controller')) {
$controller = $route->getDefault('_controller');
// If $controller is in class::method notation already skip it for performance.
if ($controller && false === strpos($controller, '::')) {

This comment has been minimized.

Copy link
@dunglas

dunglas Nov 10, 2016

Member

What do you think about using a guard clause here to reduce the code's complexity?

if (!$controller || false !== strpos($controller, '::')) {
    continue;
}

This comment has been minimized.

Copy link
@enumag

enumag Nov 10, 2016

Author Contributor

@dunglas Good idea. I'll change it.

This comment has been minimized.

Copy link
@enumag

enumag Nov 10, 2016

Author Contributor

Done.

@dunglas

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

👍

@enumag enumag force-pushed the enumag:patch-18 branch from 08cc54f to e571f99 Nov 10, 2016

@fabpot fabpot removed this from the 3.2 milestone Nov 16, 2016

@enumag enumag force-pushed the enumag:patch-18 branch from e571f99 to a588222 Nov 19, 2016

@enumag

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2016

I didn't notice the coding style error before. It's fixed now. Anything else?

// unable to optimize unknown notation
}
$controller = $route->getDefault('_controller');
// If $controller is in class::method notation already skip it for performance.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Nov 25, 2016

Member

I know I suggested this change, but looking at it again, there is a potential BC break here since we have a new case where we won't call the parser. Let's be safe and revert (still keep the typo fix above). Most of the perf benefit in this PR doesn't come from this part of the patch.

This comment has been minimized.

Copy link
@enumag

enumag Nov 25, 2016

Author Contributor

@nicolas-grekas Understood, done.

@enumag enumag force-pushed the enumag:patch-18 branch from a588222 to cf333f3 Nov 25, 2016

@stof

This comment has been minimized.

Copy link
Member

commented Nov 25, 2016

👍 for this

And should we apply it to older branches too ? We did it in the past for some performance improvements.

On a side note, this will not improve the performance in prod, except for the cache warmup, as the routing loader is not called once anymore the matcher and generator are cached.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Nov 25, 2016

Thank you @enumag.

nicolas-grekas added a commit that referenced this pull request Nov 25, 2016

bug #20374 [FrameworkBundle] Improve performance of ControllerNamePar…
…ser (enumag)

This PR was merged into the 2.7 branch.

Discussion
----------

[FrameworkBundle] Improve performance of ControllerNameParser

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Today I was searching for bottlenecks in my application using Blackfire. And among other things I found one in Symfony. Blackfire showed that `Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser::findAlternative()` was called almost 300 times which took 28 miliseconds.

It turns out that `Symfony\Bundle\FrameworkBundle\Routing\DelegatingLoader::load()` is calling `ControllerNameParser::parse()` without actually needing to do so because `$controller` is in the class::method notation already. `ControllerNameParser` threw an exception, DelegatingLoader caught and ignored it - that's ok. The problem is that generating the exception message took a lot of time because findAlternative is slow. In my case it called the levenshtein function over 5000 times which was completely useless because the exception is ignored anyway.

Commits
-------

cf333f3 [FrameworkBundle] Improve performance of ControllerNameParser
@fabpot

This comment has been minimized.

Copy link
Member

commented Nov 25, 2016

This one should not have been merged in 2.7. It does not fix a bug. This should have been merged in master instead (not even 3.2 at this stage).

@fabpot fabpot referenced this pull request Nov 27, 2016

Merged

Release v3.2.0-RC2 #20649

This was referenced Dec 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.