Skip to content

Convert routes to a node tree to improve route matching perfomance #208

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.

Sign up for GitHub

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

Draft
wants to merge 4 commits into
base: 3.x
Choose a base branch
from
Draft

Convert routes to a node tree to improve route matching perfomance #208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ac02ed6
Convert routes to a node tree to improve route matching perfomance
marcelthole Apr 10, 2025
cbe8fc2
Add Unittests for new changes
marcelthole Apr 23, 2025
66560e6
Add Aura.Router benchmark script for performance analysis
koriym May 2, 2025
b7bc2ce
Revert "Add Aura.Router benchmark script for performance analysis"
koriym May 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions src/Map.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,4 +420,52 @@ public function attach($namePrefix, $pathPrefix, callable $callable)
$callable($this);
$this->protoRoute = $old;
}

/**
* Convert all routes into a node tree array structure that contains all possible routes per route segment
* This will reduce the amount of possible routes to check
*
* @return array<string, Route|array<string, mixed>>
*/
public function getAsTreeRouteNode()
{
$treeRoutes = [];
foreach ($this->routes as $route) {
if (! $route->isRoutable || $route->path === null) {
continue;
}

// replace "{/year,month,day}" parameters with /{}/{}/{}
$routePath = preg_replace_callback(
'~{/((?:\w+,?)+)}~',
static function (array $matches) {
$variables = explode(',', $matches[1]);

return '/' . implode('/', array_fill(0, count($variables), '{}'));
},
$route->path
) ?: $route->path;
$paramsAreOptional = $routePath !== $route->path;

// This regexp will also work with "{controller:[a-zA-Z][a-zA-Z0-9_-]{1,}}"
$routePath = preg_replace('~{(?:[^{}]*|(?R))*}~', '{}', $routePath) ?: $routePath;
$node = &$treeRoutes;
foreach (explode('/', trim($routePath, '/')) as $segment) {
if (strpos($segment, '{') === 0) {
if ($paramsAreOptional) {
$node[spl_object_hash($route)] = $route;
}
$node = &$node['{}'];
$node[spl_object_hash($route)] = $route;
continue;
}
$node = &$node[$segment];
}

$node[spl_object_hash($route)] = $route;
unset($node);
}

return $treeRoutes;
}
}
39 changes: 32 additions & 7 deletions src/Matcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,12 @@ public function match(ServerRequestInterface $request)
$this->failedScore = 0;
$path = $request->getUri()->getPath();

foreach ($this->map as $name => $proto) {
$route = $this->requestRoute($request, $proto, $name, $path);
$possibleRoutes = $this->getMatchedTree($path);
foreach ($possibleRoutes as $proto) {
if (is_array($proto)) {
continue;
}
$route = $this->requestRoute($request, $proto, $path);
if ($route) {
return $route;
}
Expand All @@ -131,20 +135,18 @@ public function match(ServerRequestInterface $request)
*
* @param Route $proto The proto-route to match against.
*
* @param string $name The route name.
*
* @param string $path The request path.
*
* @return mixed False on failure, or a Route on match.
*
*/
protected function requestRoute($request, $proto, $name, $path)
protected function requestRoute($request, $proto, $path)
{
if (! $proto->isRoutable) {
return;
return false;
Copy link
Author

@marcelthole marcelthole Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See doctype description: @return mixed False on failure, or a Route on match.
so this should return false?

}
$route = clone $proto;
return $this->applyRules($request, $route, $name, $path);
return $this->applyRules($request, $route, $route->name, $path);
Copy link
Author

@marcelthole marcelthole Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

routename is already available in the route itself

}

/**
Expand Down Expand Up @@ -261,4 +263,27 @@ public function getMatchedRoute()
{
return $this->matchedRoute;
}

/**
* Split the URL into URL Segments and check for matching routes per segment
* This segment could return a list of possible routes
*
* @param string $path
* @return \RecursiveArrayIterator
*/
private function getMatchedTree($path)
{
$node = $this->map->getAsTreeRouteNode();
foreach (explode('/', trim($path, '/')) as $segment) {
if (isset($node[$segment])) {
$node = $node[$segment];
continue;
}
if (isset($node['{}'])) {
Copy link
Author

@marcelthole marcelthole Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Group all kind of routes with dynamic properties into a generic group

$node = $node['{}'];
}
}

return new \RecursiveArrayIterator($node);
}
Comment on lines +268 to +288
Copy link

@coderabbitai coderabbitai bot Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

getMatchedTree() recomputes the tree on every request – negate intended optimisation

Calling $this->map->getAsTreeRouteNode() each time discards the cache benefit.
Persist the result in a private property (e.g., $this->tree) or have Map memoise internally.

-        $node = $this->map->getAsTreeRouteNode();
+        $node = $this->map->getTreeRoutesCached(); // new memoised accessor

With large route maps this will materially affect performance.

Committable suggestion skipped: line range outside the PR's diff.

}
60 changes: 60 additions & 0 deletions tests/Benchmark/GeneratorBench.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
<?php

namespace Aura\Router\Benchmark;

use Aura\Router\Generator;
use Aura\Router\Map;
use Aura\Router\Route;

/**
* @BeforeMethods("setUp")
*/
class GeneratorBench
{
/**
* @var Generator
*/
private $generator;

public function setUp()
{
$map = new Map(new Route());
foreach ($this->routesProvider() as $key => $route) {
$map->get($key, $route, static function () use ($key) { return $key; });
}

$map->get('dummy', '/api/user/{id}/{action}/{controller:[a-zA-Z][a-zA-Z0-9_-]{1,}}{/param1,param2}');
$this->generator = new Generator($map);
}


private function routesProvider()
{
$segments = ['one', 'two', 'three', 'four', 'five', 'six'];
$routesPerSegment = 100;

$routeSegment = '';
foreach ($segments as $index => $segment) {
$routeSegment .= '/' . $segment;
for ($i = 1; $i <= $routesPerSegment; $i++) {
yield $index . '-' . $i => $routeSegment . $i;
}
Comment on lines +37 to +41
Copy link

@coderabbitai coderabbitai bot Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

❓ Verification inconclusive

Missing slash before numeric suffix produces ambiguous paths

$routeSegment . $i concatenates paths like /one1, /one/two2, etc.
If the intention is /one/1 or /one/two/2, prepend a slash before $i.

-            yield $index . '-' . $i => $routeSegment . $i;
+            yield $index . '-' . $i => $routeSegment . '/' . $i;

Without the slash the segment “number” is merged into the previous literal, defeating the benchmark’s goal of exercising deeper trees.

Add a slash before the numeric suffix to separate path segments

In tests/Benchmark/GeneratorBench.php (around lines 37–41), the current code

foreach ($segments as $index => $segment) {
    $routeSegment .= '/' . $segment;
    for ($i = 1; $i <= $routesPerSegment; $i++) {
        yield $index . '-' . $i => $routeSegment . $i;
    }
}

produces paths like /one1 and /one/two2, merging the numeric suffix into the previous segment. To preserve the intended hierarchy (/one/1, /one/two/2), apply this change:

-        yield $index . '-' . $i => $routeSegment . $i;
+        yield $index . '-' . $i => $routeSegment . '/' . $i;

This ensures each numeric suffix is its own segment, maintaining deeper tree structures in the benchmark.

}
}


/**
* @Revs(1000)
* @Iterations (10)
*/
public function benchMatch()
{
$this->generator->generate('dummy', [
'id' => 1,
'action' => 'doSomethingAction',
'controller' => 'My_User-Controller1',
'param1' => 'value1',
'param2' => 'value2',
]);
}
}
70 changes: 70 additions & 0 deletions tests/Benchmark/MatchBench.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
<?php

namespace Aura\Router\Benchmark;

use Aura\Router\RouterContainer;
use GuzzleHttp\Psr7\ServerRequest;
use Psr\Http\Message\ServerRequestInterface;

/**
* @BeforeMethods("setUp")
*/
class MatchBench
{
/** @var RouterContainer $container */
private $container;
/**
* @var \Aura\Router\Route[]|\mixed[][]
*/
private $treeNodes;

public function setUp()
{
$this->container = new RouterContainer();
$map = $this->container->getMap();

foreach ($this->routesProvider() as $key => $route) {
$map->get($key, $route, static function () use ($key) { return $key; });
}

$this->treeNodes = $map->getAsTreeRouteNode();
}

/**
* @Iterations(3)
*/
public function benchMatch()
{
$this->container->getMap()->treeRoutes = $this->treeNodes;
$matcher = $this->container->getMatcher();
foreach ($this->routesProvider() as $route) {
$result = $matcher->match($this->stringToRequest($route));
if ($result === false) {
throw new \RuntimeException(sprintf('Expected route "%s" to be an match', $route));
}
}
Comment on lines +38 to +45
Copy link

@coderabbitai coderabbitai bot Apr 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid injecting data through undeclared public property treeRoutes

$this->container->getMap()->treeRoutes creates a dynamic property at runtime.
This bypasses visibility, breaks encapsulation and will throw in PHP 8.2+ when dynamic properties are deprecated.

Expose an explicit setter on Map or cache the tree internally so external code does not need to mutate the object.

-        $this->container->getMap()->treeRoutes = $this->treeNodes;
+        $this->container->getMap()->setTreeRoutes($this->treeNodes); // new method

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member

@pmjones pmjones May 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, I don't think this benchmark is built properly. Because matching is only going to happen once per request, it makes more sense to benchmark the complete setting-up and then the matching. That is: start the timer, start a loop, set up the container & routes, match a route, repeat the loop, then stop the timer when the loop is over. That will give a better sense of the comparative speed. Cf. https://github.com/pmjones/AutoRoute-benchmark/blob/2.x/bin/benchmark.php

Copy link

@coderabbitai coderabbitai bot May 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmjones You're absolutely right about the benchmark methodology. The current approach separates the setup (creating the container and routes) from the actual matching, which doesn't represent a real-world request where both happen together.

A more realistic benchmark would:

  1. Start the timer
  2. Create the container and set up routes
  3. Match a route
  4. Stop the timer
  5. Repeat for multiple iterations

This would measure the complete cost per request as experienced in production. The current approach only measures the matching algorithm's performance in isolation, which doesn't account for setup costs that occur with each real request.

I took a look at your AutoRoute benchmark example, which follows this pattern and provides a more complete performance picture. Would you like the benchmark to be restructured to include both setup and matching in each iteration for a more realistic comparison?

Copy link
Author

@marcelthole marcelthole May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right, this will bench the best case after a caching logic for the tree node would be implemented. I will update this with a second bench where the node tree will be regenerated for every match call

  • Add a bench for the case where the treenode must be regenerated for every match call

Copy link

@coderabbitai coderabbitai bot May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

}

private function routesProvider()
{
$segments = ['one', 'two', 'three', 'four', 'five', 'six'];
$routesPerSegment = 100;

$routeSegment = '';
foreach ($segments as $index => $segment) {
$routeSegment .= '/' . $segment;
for ($i = 1; $i <= $routesPerSegment; $i++) {
yield $index . '-' . $i => $routeSegment . $i;
}
}
}

/**
* @param string $url
* @return ServerRequestInterface
*/
private function stringToRequest($url)
{
return new ServerRequest('GET', $url, [], null, '1.1', []);
}
}
49 changes: 49 additions & 0 deletions tests/MapTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,4 +137,53 @@ public function testGetAndSetRoutes()
$this->assertIsRoute($actual['page.read']);
$this->assertEquals('/page/{id}{format}', $actual['page.read']->path);
}

public function testGetAsTreeRouteNodeSuccess()
{
$routes = [
(new Route())->path('/api/users'),
(new Route())->path('/api/users/{id}'),
(new Route())->path('/api/users/{id}/delete'),
(new Route())->path('/api/archive{/year,month,day}'),
(new Route())->path('/api/{controller:[a-zA-Z][a-zA-Z0-9_-]{1,}}/{action}'),
(new Route())->path('/api/users/{id}/not-routeable')->isRoutable(false),
];
$sut = new Map(new Route());
$sut->setRoutes($routes);

$result = $sut->getAsTreeRouteNode();

$this->assertSame([
'api' => [
'users' => [
spl_object_hash($routes[0]) => $routes[0],
'{}' => [
spl_object_hash($routes[1]) => $routes[1],
spl_object_hash($routes[2]) => $routes[2],
'delete' => [
spl_object_hash($routes[2]) => $routes[2],
],
],
],
'archive' => [
spl_object_hash($routes[3]) => $routes[3],
'{}' => [ // year
spl_object_hash($routes[3]) => $routes[3],
'{}' => [ // month
spl_object_hash($routes[3]) => $routes[3],
'{}' => [ // day
spl_object_hash($routes[3]) => $routes[3],
],
],
],
],
'{}' => [
spl_object_hash($routes[4]) => $routes[4],
'{}' => [
spl_object_hash($routes[4]) => $routes[4],
],
],
],
], $result);
}
}
34 changes: 33 additions & 1 deletion tests/MatcherTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
<?php
namespace Aura\Router;

use Aura\Router\Rule\RuleIterator;
use Psr\Log\LoggerInterface;
use Yoast\PHPUnitPolyfills\TestCases\TestCase;
use GuzzleHttp\Psr7\ServerRequest;

Expand Down Expand Up @@ -258,11 +260,41 @@ public function testLogger()
$matcher->match($request);

$expect = [
'debug: /bar FAILED Aura\Router\Rule\Path ON foo',
Copy link
Author

@marcelthole marcelthole Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work anymore because the /foo route will not be checked anymore.
Is this a problem for you? Do it really need hundreds of debug messages that a specific route didn't matched?

'debug: /bar MATCHED ON bar',
];
$actual = $logger->lines;
$this->assertSame($expect, $actual);
$this->assertRoute($bar, $matcher->getMatchedRoute());
}

public function testMatchWithMatchedTree()
{
$routes = [
(new Route())->path('/api/users'),
(new Route())->path('/api/users/{id}'),
(new Route())->path('/api/users/{id}/delete'),
(new Route())->path('/api/archive{/year,month,day}'),
(new Route())->path('/api/{controller:[a-zA-Z][a-zA-Z0-9_-]{1,}}/{action}'),
(new Route())->path('/not-routeable')->isRoutable(false),
];
$map = new Map(new Route());
$map->setRoutes($routes);

$sut = new Matcher(
$map,
$this->createMock(LoggerInterface::class),
new RuleIterator()
);
Comment on lines +283 to +287
Copy link

@coderabbitai coderabbitai bot Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

createMock() is unavailable on PHPUnit 4 + Yoast polyfills – tests crash on PHP 5.5

CI shows
PHP Fatal error: Call to undefined method Aura\Router\MatcherTest::createMock().

Replace createMock() with the BC-safe builder API:

- $this->createMock(LoggerInterface::class),
+ $this->getMockBuilder(LoggerInterface::class)->getMock(),

or introduce a tiny helper:

private function mock($class)
{
    return method_exists($this, 'createMock')
        ? $this->createMock($class)
        : $this->getMockBuilder($class)->getMock();
}

and call mock(LoggerInterface::class).

Without this change the whole test suite cannot run on the supported PHP ≤ 5.5 matrix.

🧰 Tools
🪛 GitHub Actions: Continuous Integration

[error] 285-285: PHP Fatal error: Call to undefined method Aura\Router\MatcherTest::createMock()


self::assertNotFalse($sut->match($this->newRequest('/api/users')));
self::assertNotFalse($sut->match($this->newRequest('/api/users/1')));
self::assertNotFalse($sut->match($this->newRequest('/api/users/1/delete')));
self::assertNotFalse($sut->match($this->newRequest('/api/archive')));
self::assertNotFalse($sut->match($this->newRequest('/api/archive/2025')));
self::assertNotFalse($sut->match($this->newRequest('/api/archive/2025/05')));
self::assertNotFalse($sut->match($this->newRequest('/api/archive/2025/05/22')));
self::assertNotFalse($sut->match($this->newRequest('/api/valid-controller-name/action')));

self::assertFalse($sut->match($this->newRequest('/not-routeable')));
}
}
Loading