bug symfony#20374 [FrameworkBundle] Improve performance of ControllerNameParser (enumag)

nicolas-grekas · nicolas-grekas · commit e62b602dc446 · 2016-11-25T13:12:15.000+01:00
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
diff --git a/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerNameParser.php b/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerNameParser.php
@@ -46,11 +46,12 @@ public function __construct(KernelInterface $kernel)
      */
     public function parse($controller)
     {
-        $originalController = $controller;
-        if (3 !== count($parts = explode(':', $controller))) {
+        $parts = explode(':', $controller);
+        if (3 !== count($parts) || in_array('', $parts, true)) {
             throw new \InvalidArgumentException(sprintf('The "%s" controller is not a valid "a:b:c" controller string.', $controller));
         }
 
+        $originalController = $controller;
         list($bundle, $controller, $action) = $parts;
         $controller = str_replace('/', '\\', $controller);
         $bundles = array();
diff --git a/src/Symfony/Bundle/FrameworkBundle/Routing/DelegatingLoader.php b/src/Symfony/Bundle/FrameworkBundle/Routing/DelegatingLoader.php
@@ -21,7 +21,7 @@
  * 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,15 +85,17 @@ public function load($resource, $type = null)
         $this->loading = false;
 
         foreach ($collection->all() as $route) {
-            if ($controller = $route->getDefault('_controller')) {
-                try {
-                    $controller = $this->parser->parse($controller);
-                } catch (\InvalidArgumentException $e) {
-                    // unable to optimize unknown notation
-                }
+            if (!$controller = $route->getDefault('_controller')) {
+                continue;
+            }
 
-                $route->setDefault('_controller', $controller);
+            try {
+                $controller = $this->parser->parse($controller);
+            } catch (\InvalidArgumentException $e) {
+                // unable to optimize unknown notation
             }
+
+            $route->setDefault('_controller', $controller);
         }
 
         return $collection;

Original file line number	Diff line number	Diff line change
`@@ -46,11 +46,12 @@ public function __construct(KernelInterface $kernel)`
`46`	`46`	`*/`
`47`	`47`	`public function parse($controller)`
`48`	`48`	`{`
`49`		`- $originalController = $controller;`
`50`		`- if (3 !== count($parts = explode(':', $controller))) {`
	`49`	`+ $parts = explode(':', $controller);`
	`50`	`+ if (3 !== count($parts) \|\| in_array('', $parts, true)) {`
`51`	`51`	`throw new \InvalidArgumentException(sprintf('The "%s" controller is not a valid "a:b:c" controller string.', $controller));`
`52`	`52`	`}`
`53`	`53`
	`54`	`+ $originalController = $controller;`
`54`	`55`	`list($bundle, $controller, $action) = $parts;`
`55`	`56`	`$controller = str_replace('/', '\\', $controller);`
`56`	`57`	`$bundles = array();`