Skip to content

Commit

Permalink
bug #26208 [Routing] Fix same-prefix aggregation (nicolas-grekas)
Browse files Browse the repository at this point in the history 
This PR was merged into the 4.1-dev branch.

Discussion
----------

[Routing] Fix same-prefix aggregation

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

The blog post on http://symfony.com/blog/new-in-symfony-4-1-fastest-php-router made me review the aggregation logic, and discover issues.

So now, instead of this:
```
        .'|/(en|fr)/admin/post/?(*:82)'
        .'|/(en|fr)/admin/post/new(*:166)'
        .'|/(en|fr)/admin/post/(\\d+)(*:253)'
        .'|/(en|fr)/admin/post/(\\d+)/edit(*:345)'
        .'|/(en|fr)/admin/post/(\\d+)/delete(*:442)'
        .'|/(en|fr)/blog/?(*:519)'
        .'|/(en|fr)/blog/rss\\.xml(*:603)'
        .'|/(en|fr)/blog/page/([^/]++)(*:694)'
        .'|/(en|fr)/blog/posts/([^/]++)(*:784)'
        .'|/(en|fr)/blog/comment/(\d+)/new(*:880)'
        .'|/(en|fr)/blog/search(*:962)'
        .'|/(en|fr)/login(*:1038)'
        .'|/(en|fr)/logout(*:1116)'
        .'|/(en|fr)?(*:1188)'
```

we generate this:
```
        .'|/(en|fr)(?'
            .'|/admin/post(?'
                .'|/?(*:34)'
                .'|/new(*:45)'
                .'|/(\\d+)(?'
                    .'|(*:61)'
                    .'|/edit(*:73)'
                    .'|/delete(*:87)'
                .')'
            .')'
            .'|/blog(?'
                .'|/?(*:106)'
                .'|/rss\\.xml(*:123)'
                .'|/p(?'
                    .'|age/([^/]++)(*:148)'
                    .'|osts/([^/]++)(*:169)'
                .')'
                .'|/comments/(\\d+)/new(*:197)'
                .'|/search(*:212)'
            .')'
            .'|/log(?'
                .'|in(*:230)'
                .'|out(*:241)'
            .')'
        .')'
        .'|/(en|fr)?(*:260)'
```

This is not only another perf fix, but a real bug fix, as I found ordering issues.

Commits
-------

d514f81 [Routing] Fix same-prefix aggregation
  • Loading branch information
@fabpot
fabpot committed Feb 19, 2018
2 parents b0facfe + d514f81 commit b1f45b3
Show file tree
Hide file tree
Showing 7 changed files with 250 additions and 96 deletions.
8 changes: 4 additions & 4 deletions src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
View file
Expand Up @@ -97,10 +97,10 @@ private function generateMatchMethod(bool $supportsRedirections): string
foreach ($this->getRoutes()->all() as $name => $route) {
if ($host = $route->getHost()) {
$matchHost = true;
$host = '/'.str_replace('.', '/', rtrim(explode('}', strrev($host), 2)[0], '.'));
$host = '/'.strtr(strrev($host), '}.{', '(/)');
}

$routes->addRoute($host ?: '/', array($name, $route));
$routes->addRoute($host ?: '/(.*)', array($name, $route));
}
$routes = $matchHost ? $routes->populateCollection(new RouteCollection()) : $this->getRoutes();

Expand Down Expand Up @@ -139,7 +139,7 @@ private function compileRoutes(RouteCollection $routes, bool $supportsRedirectio

while (true) {
try {
$this->signalingException = new \RuntimeException('PCRE compilation failed: regular expression is too large');
$this->signalingException = new \RuntimeException('preg_match(): Compilation failed: regular expression is too large');
$code .= $this->compileDynamicRoutes($dynamicRoutes, $supportsRedirections, $matchHost, $chunkLimit);
break;
} catch (\Exception $e) {
Expand Down Expand Up @@ -377,7 +377,7 @@ private function compileDynamicRoutes(RouteCollection $collection, bool $support
$state->regex .= $rx;

// if the regex is too large, throw a signaling exception to recompute with smaller chunk size
set_error_handler(function ($type, $message) { throw $this->signalingException; });
set_error_handler(function ($type, $message) { throw 0 === strpos($message, $this->signalingException->getMessage()) ? $this->signalingException : new \ErrorException($message); });
try {
preg_match($state->regex, '');
} finally {
Expand Down
100 changes: 51 additions & 49 deletions src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php
View file
Expand Up @@ -17,14 +17,18 @@
* Prefix tree of routes preserving routes order.
*
* @author Frank de Jonge <info@frankdejonge.nl>
* @author Nicolas Grekas <p@tchwork.com>
*
* @internal
*/
class StaticPrefixCollection
{
private $prefix;
private $staticPrefix;
private $matchStart = 0;

/**
* @var string[]
*/
private $staticPrefixes = array();

/**
* @var string[]
Expand All @@ -36,10 +40,9 @@ class StaticPrefixCollection
*/
private $items = array();

public function __construct(string $prefix = '/', string $staticPrefix = '/')
public function __construct(string $prefix = '/')
{
$this->prefix = $prefix;
$this->staticPrefix = $staticPrefix;
}

public function getPrefix(): string
Expand All @@ -60,41 +63,38 @@ public function getRoutes(): array
*
* @param array|self $route
*/
public function addRoute(string $prefix, $route)
public function addRoute(string $prefix, $route, string $staticPrefix = null)
{
$this->guardAgainstAddingNotAcceptedRoutes($prefix);
list($prefix, $staticPrefix) = $this->detectCommonPrefix($prefix, $prefix) ?: array(rtrim($prefix, '/') ?: '/', '/');

if ($this->staticPrefix === $staticPrefix) {
// When a prefix is exactly the same as the base we move up the match start position.
// This is needed because otherwise routes that come afterwards have higher precedence
// than a possible regular expression, which goes against the input order sorting.
$this->prefixes[] = $prefix;
$this->items[] = $route;
$this->matchStart = count($this->items);

return;
if (null === $staticPrefix) {
list($prefix, $staticPrefix) = $this->getCommonPrefix($prefix, $prefix);
}

for ($i = $this->matchStart; $i < \count($this->items); ++$i) {
for ($i = \count($this->items) - 1; 0 <= $i; --$i) {
$item = $this->items[$i];

if ($item instanceof self && $item->accepts($prefix)) {
$item->addRoute($prefix, $route);
$item->addRoute($prefix, $route, $staticPrefix);

return;
}

if ($group = $this->groupWithItem($i, $prefix, $route)) {
$this->prefixes[$i] = $group->getPrefix();
$this->items[$i] = $group;

if ($this->groupWithItem($i, $prefix, $staticPrefix, $route)) {
return;
}

if ($this->staticPrefixes[$i] !== $this->prefixes[$i] && 0 === strpos($staticPrefix, $this->staticPrefixes[$i])) {
break;
}

if ($staticPrefix !== $prefix && 0 === strpos($this->staticPrefixes[$i], $staticPrefix)) {
break;
}
}

// No optimised case was found, in this case we simple add the route for possible
// grouping when new routes are added.
$this->staticPrefixes[] = $staticPrefix;
$this->prefixes[] = $prefix;
$this->items[] = $route;
}
Expand All @@ -118,44 +118,44 @@ public function populateCollection(RouteCollection $routes): RouteCollection
/**
* Tries to combine a route with another route or group.
*/
private function groupWithItem(int $i, string $prefix, $route): ?self
private function groupWithItem(int $i, string $prefix, string $staticPrefix, $route): bool
{
if (!$commonPrefix = $this->detectCommonPrefix($prefix, $this->prefixes[$i])) {
return null;
list($commonPrefix, $commonStaticPrefix) = $this->getCommonPrefix($prefix, $this->prefixes[$i]);

if (\strlen($this->prefix) >= \strlen($commonPrefix)) {
return false;
}

$child = new self(...$commonPrefix);
$item = $this->items[$i];
$child = new self($commonPrefix);

if ($item instanceof self) {
$child->prefixes = array($commonPrefix[0]);
$child->items = array($item);
} else {
$child->addRoute($this->prefixes[$i], $item);
}
$child->staticPrefixes = array($this->staticPrefixes[$i], $staticPrefix);
$child->prefixes = array($this->prefixes[$i], $prefix);
$child->items = array($this->items[$i], $route);

$child->addRoute($prefix, $route);
$this->staticPrefixes[$i] = $commonStaticPrefix;
$this->prefixes[$i] = $commonPrefix;
$this->items[$i] = $child;

return $child;
return true;
}

/**
* Checks whether a prefix can be contained within the group.
*/
private function accepts(string $prefix): bool
{
return '' === $this->prefix || 0 === strpos($prefix, $this->prefix);
return 0 === strpos($prefix, $this->prefix) && '?' !== ($prefix[\strlen($this->prefix)] ?? '');
}

/**
* Detects whether there's a common prefix relative to the group prefix and returns it.
* Gets the full and static common prefixes between two route patterns.
*
* @return null|array A common prefix, longer than the base/group prefix, or null when none available
* The static prefix stops at last at the first opening bracket.
*/
private function detectCommonPrefix(string $prefix, string $anotherPrefix): ?array
private function getCommonPrefix(string $prefix, string $anotherPrefix): array
{
$baseLength = strlen($this->prefix);
$end = min(strlen($prefix), strlen($anotherPrefix));
$baseLength = \strlen($this->prefix);
$end = min(\strlen($prefix), \strlen($anotherPrefix));
$staticLength = null;

for ($i = $baseLength; $i < $end && $prefix[$i] === $anotherPrefix[$i]; ++$i) {
Expand All @@ -177,21 +177,23 @@ private function detectCommonPrefix(string $prefix, string $anotherPrefix): ?arr
if (0 < $n) {
break;
}
$i = $j;
if (('?' === ($prefix[$j] ?? '') || '?' === ($anotherPrefix[$j] ?? '')) && ($prefix[$j] ?? '') !== ($anotherPrefix[$j] ?? '')) {
break;
}
$i = $j - 1;
} elseif ('\\' === $prefix[$i] && (++$i === $end || $prefix[$i] !== $anotherPrefix[$i])) {
--$i;
break;
}
}

$staticLength = $staticLength ?? $i;
$commonPrefix = rtrim(substr($prefix, 0, $i), '/');

if (strlen($commonPrefix) > $baseLength) {
return array($commonPrefix, rtrim(substr($prefix, 0, $staticLength), '/') ?: '/');
if (1 < $i && '/' === $prefix[$i - 1]) {
--$i;
}
if (null !== $staticLength && 1 < $staticLength && '/' === $prefix[$staticLength - 1]) {
--$staticLength;
}

return null;
return array(substr($prefix, 0, $i), substr($prefix, 0, $staticLength ?? $i));
}

/**
Expand Down
44 changes: 23 additions & 21 deletions src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php
View file
Expand Up @@ -98,23 +98,25 @@ public function match($rawPathinfo)
.')'
.')'
.'|/multi/hello(?:/([^/]++))?(*:238)'
.'|/([^/]++)/b/([^/]++)(*:266)'
.'|/([^/]++)/b/([^/]++)(*:294)'
.'|/aba/([^/]++)(*:315)'
.'|/([^/]++)/b/([^/]++)(?'
.'|(*:269)'
.'|(*:277)'
.')'
.'|/aba/([^/]++)(*:299)'
.')|(?i:([^\\.]++)\\.example\\.com)(?'
.'|/route1(?'
.'|3/([^/]++)(*:375)'
.'|4/([^/]++)(*:393)'
.'|3/([^/]++)(*:359)'
.'|4/([^/]++)(*:377)'
.')'
.')|(?i:c\\.example\\.com)(?'
.'|/route15/([^/]++)(*:443)'
.'|/route15/([^/]++)(*:427)'
.')|[^/]*+(?'
.'|/route16/([^/]++)(*:478)'
.'|/route16/([^/]++)(*:462)'
.'|/a(?'
.'|/a\\.\\.\\.(*:499)'
.'|/a\\.\\.\\.(*:483)'
.'|/b(?'
.'|/([^/]++)(*:521)'
.'|/c/([^/]++)(*:540)'
.'|/([^/]++)(*:505)'
.'|/c/([^/]++)(*:524)'
.')'
.')'
.')'
Expand Down Expand Up @@ -172,7 +174,7 @@ public function match($rawPathinfo)
return $this->mergeDefaults(array_replace($matches, array('_route' => 'foo2')), array());

break;
case 266:
case 269:
$matches = array('_locale' => $matches[1] ?? null, 'foo' => $matches[2] ?? null);

// foo3
Expand All @@ -189,15 +191,15 @@ public function match($rawPathinfo)
170 => array(array('_route' => 'overridden'), array('var'), null, null),
202 => array(array('_route' => 'bar2'), array('bar1'), null, null),
238 => array(array('_route' => 'helloWorld', 'who' => 'World!'), array('who'), null, null),
294 => array(array('_route' => 'bar3'), array('_locale', 'bar'), null, null),
315 => array(array('_route' => 'foo4'), array('foo'), null, null),
375 => array(array('_route' => 'route13'), array('var1', 'name'), null, null),
393 => array(array('_route' => 'route14', 'var1' => 'val'), array('var1', 'name'), null, null),
443 => array(array('_route' => 'route15'), array('name'), null, null),
478 => array(array('_route' => 'route16', 'var1' => 'val'), array('name'), null, null),
499 => array(array('_route' => 'a'), array(), null, null),
521 => array(array('_route' => 'b'), array('var'), null, null),
540 => array(array('_route' => 'c'), array('var'), null, null),
277 => array(array('_route' => 'bar3'), array('_locale', 'bar'), null, null),
299 => array(array('_route' => 'foo4'), array('foo'), null, null),
359 => array(array('_route' => 'route13'), array('var1', 'name'), null, null),
377 => array(array('_route' => 'route14', 'var1' => 'val'), array('var1', 'name'), null, null),
427 => array(array('_route' => 'route15'), array('name'), null, null),
462 => array(array('_route' => 'route16', 'var1' => 'val'), array('name'), null, null),
483 => array(array('_route' => 'a'), array(), null, null),
505 => array(array('_route' => 'b'), array('var'), null, null),
524 => array(array('_route' => 'c'), array('var'), null, null),
);

list($ret, $vars, $requiredMethods, $requiredSchemes) = $routes[$m];
Expand All @@ -216,7 +218,7 @@ public function match($rawPathinfo)
return $ret;
}

if (540 === $m) {
if (524 === $m) {
break;
}
$regex = substr_replace($regex, 'F', $m - $offset, 1 + strlen($m));
Expand Down

0 comments on commit b1f45b3

Please sign in to comment.