minor #24556 [Routing] Ensure uniqueness without repeated check (endroid)

fabpot · fabpot · commit f757cd65e262 · 2017-10-16T13:33:36.000-07:00
This PR was squashed before being merged into the 3.4 branch (closes #24556). Discussion ---------- [Routing] Ensure uniqueness without repeated check | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - The RouteCollection ensures uniqueness of resources by using array_unique. The current implementation suffers from the following problems. 1. The array_unique method calls __toString for every element in the array but it is very inefficient in terms of the number of calls made. I.e. if the array has 50 elements it does not do 50 __toString calls. I did some tests with Blackfire in PHP 7.1 and found the following numbers. 25 elements => 240 calls 50 elements => 607 calls 100 elements => 1382 calls 200 elements => 3333 calls 2. The array_unique function is called every time the getResources() method is called, even when the resources did not change in the mean time. Combined with the above this leads to lower performance. Many applications have a low number of routes so this is not a big issue. But for larger applications or bundles that generate routes (i.e. for CRUD or API / doc generation) this will have a bigger impact. Commits ------- 16f7281 [Routing] Ensure uniqueness without repeated check
diff --git a/src/Symfony/Component/Routing/RouteCollection.php b/src/Symfony/Component/Routing/RouteCollection.php
@@ -128,7 +128,9 @@ public function addCollection(RouteCollection $collection)
             $this->routes[$name] = $route;
         }
 
-        $this->resources = array_merge($this->resources, $collection->getResources());
+        foreach ($collection->getResources() as $resource) {
+            $this->addResource($resource);
+        }
     }
 
     /**
@@ -262,16 +264,21 @@ public function setMethods($methods)
      */
     public function getResources()
     {
-        return array_unique($this->resources);
+        return array_values($this->resources);
     }
 
     /**
-     * Adds a resource for this collection.
+     * Adds a resource for this collection. If the resource already exists
+     * it is not added.
      *
      * @param ResourceInterface $resource A resource instance
      */
     public function addResource(ResourceInterface $resource)
     {
-        $this->resources[] = $resource;
+        $key = (string) $resource;
+
+        if (!isset($this->resources[$key])) {
+            $this->resources[$key] = $resource;
+        }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,9 @@ public function addCollection(RouteCollection $collection)`
`128`	`128`	`$this->routes[$name] = $route;`
`129`	`129`	`}`
`130`	`130`
`131`		`- $this->resources = array_merge($this->resources, $collection->getResources());`
	`131`	`+ foreach ($collection->getResources() as $resource) {`
	`132`	`+ $this->addResource($resource);`
	`133`	`+ }`
`132`	`134`	`}`
`133`	`135`
`134`	`136`	`/**`
`@@ -262,16 +264,21 @@ public function setMethods($methods)`
`262`	`264`	`*/`
`263`	`265`	`public function getResources()`
`264`	`266`	`{`
`265`		`- return array_unique($this->resources);`
	`267`	`+ return array_values($this->resources);`
`266`	`268`	`}`
`267`	`269`
`268`	`270`	`/**`
`269`		`- * Adds a resource for this collection.`
	`271`	`+ * Adds a resource for this collection. If the resource already exists`
	`272`	`+ * it is not added.`
`270`	`273`	`*`
`271`	`274`	`* @param ResourceInterface $resource A resource instance`
`272`	`275`	`*/`
`273`	`276`	`public function addResource(ResourceInterface $resource)`
`274`	`277`	`{`
`275`		`- $this->resources[] = $resource;`
	`278`	`+ $key = (string) $resource;`
	`279`	`+`
	`280`	`+ if (!isset($this->resources[$key])) {`
	`281`	`+ $this->resources[$key] = $resource;`
	`282`	`+ }`
`276`	`283`	`}`
`277`	`284`	`}`