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.

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

Jump to bottom

[Routing] Optimised dumped router matcher, prevent unneeded function calls. #21755

Closed
wants to merge 12 commits into from
Closed

[Routing] Optimised dumped router matcher, prevent unneeded function calls. #21755

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ea8c55a
Optimized dumped router matcher, prevent unneeded function calls.
frankdejonge Feb 24, 2017
0d69cfe
Fixed CS
frankdejonge Feb 24, 2017
0425f33
Flipped around special GET HEAD handling for less expensive method co…
frankdejonge Feb 25, 2017
56f2323
Corrected tests.
frankdejonge Feb 25, 2017
ef6ad3a
Use yoda statements.
frankdejonge Feb 27, 2017
44271e0
After filtering there's also a case where only one HTTP method is pre…
frankdejonge Feb 27, 2017
e4a9e1e
CS
frankdejonge Feb 27, 2017
2e180c6
Optimise schema check as well.
frankdejonge Feb 27, 2017
9d92c57
Added a test case which more clearly demonstrates the effect of the h…
frankdejonge Feb 27, 2017
f56cf9a
Yoda comparison.
frankdejonge Feb 28, 2017
bb68154
Renamed isLikeGetMethod to canonicalMethod.
frankdejonge Feb 28, 2017
4ab821b
It's scheme not schema.
frankdejonge Feb 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
View file
Open in desktop
Expand Up @@ -105,8 +105,10 @@ public function match(\$pathinfo)
{
\$allow = array();
\$pathinfo = rawurldecode(\$pathinfo);
\$trimmedPathinfo = rtrim(\$pathinfo, '/');
\$context = \$this->context;
\$request = \$this->request;
\$requestMethod = \$context->getMethod();

$code

Expand All @@ -133,7 +135,7 @@ private function compileRoutes(RouteCollection $routes, $supportsRedirections)
foreach ($groups as $collection) {
if (null !== $regex = $collection->getAttribute('host_regex')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This removes a reference lookup, probably hardly any impact, but in optimised code, every cycle counts, right?

if (!$fetchedHost) {
$code .= " \$host = \$this->context->getHost();\n\n";
$code .= " \$host = \$context->getHost();\n\n";
$fetchedHost = true;
}

Expand Down Expand Up @@ -227,7 +229,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren

if (!count($compiledRoute->getPathVariables()) && false !== preg_match('#^(.)\^(?P<url>.*?)\$\1#'.(substr($regex, -1) === 'u' ? 'u' : ''), $regex, $m)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This removed N trim calls.

if ($supportsTrailingSlash && substr($m['url'], -1) === '/') {
$conditions[] = sprintf("rtrim(\$pathinfo, '/') === %s", var_export(rtrim(str_replace('\\', '', $m['url']), '/'), true));
$conditions[] = sprintf('$trimmedPathinfo === %s', var_export(rtrim(str_replace('\\', '', $m['url']), '/'), true));
$hasTrailingSlash = true;
} else {
$conditions[] = sprintf('$pathinfo === %s', var_export(str_replace('\\', '', $m['url']), true));
Expand Down Expand Up @@ -266,7 +268,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
if ($methods) {
if (1 === count($methods)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This removes N function calls.

$code .= <<<EOF
if (\$this->context->getMethod() != '$methods[0]') {
if (\$requestMethod != '$methods[0]') {
\$allow[] = '$methods[0]';
goto $gotoname;
}
Expand All @@ -276,7 +278,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
} else {
$methods = implode("', '", $methods);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This removes N function calls.

$code .= <<<EOF
if (!in_array(\$this->context->getMethod(), array('$methods'))) {
if (!in_array(\$requestMethod, array('$methods'))) {
\$allow = array_merge(\$allow, array('$methods'));
goto $gotoname;
}
Expand All @@ -303,7 +305,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
$schemes = str_replace("\n", '', var_export(array_flip($schemes), true));
$code .= <<<EOF
\$requiredSchemes = $schemes;
if (!isset(\$requiredSchemes[\$this->context->getScheme()])) {
if (!isset(\$requiredSchemes[\$context->getScheme()])) {
return \$this->redirect(\$pathinfo, '$name', key(\$requiredSchemes));
}

Expand Down
12 changes: 7 additions & 5 deletions src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php
View file
Open in desktop
Expand Up @@ -24,8 +24,10 @@ public function match($pathinfo)
{
$allow = array();
$pathinfo = rawurldecode($pathinfo);
$trimmedPathinfo = rtrim($pathinfo, '/');
$context = $this->context;
$request = $this->request;
$requestMethod = $context->getMethod();

// foo
if (0 === strpos($pathinfo, '/foo') && preg_match('#^/foo/(?P<bar>baz|symfony)$#s', $pathinfo, $matches)) {
Expand All @@ -35,7 +37,7 @@ public function match($pathinfo)
if (0 === strpos($pathinfo, '/bar')) {
// bar
if (preg_match('#^/bar/(?P<foo>[^/]++)$#s', $pathinfo, $matches)) {
if (!in_array($this->context->getMethod(), array('GET', 'HEAD'))) {
if (!in_array($requestMethod, array('GET', 'HEAD'))) {
$allow = array_merge($allow, array('GET', 'HEAD'));
goto not_bar;
}
Expand All @@ -46,7 +48,7 @@ public function match($pathinfo)

// barhead
if (0 === strpos($pathinfo, '/barhead') && preg_match('#^/barhead/(?P<foo>[^/]++)$#s', $pathinfo, $matches)) {
if (!in_array($this->context->getMethod(), array('GET', 'HEAD'))) {
if (!in_array($requestMethod, array('GET', 'HEAD'))) {
$allow = array_merge($allow, array('GET', 'HEAD'));
goto not_barhead;
}
Expand Down Expand Up @@ -83,7 +85,7 @@ public function match($pathinfo)

// baz5
if (preg_match('#^/test/(?P<foo>[^/]++)/$#s', $pathinfo, $matches)) {
if ($this->context->getMethod() != 'POST') {
if ($requestMethod != 'POST') {
$allow[] = 'POST';
goto not_baz5;
}
Expand All @@ -94,7 +96,7 @@ public function match($pathinfo)

// baz.baz6
if (preg_match('#^/test/(?P<foo>[^/]++)/$#s', $pathinfo, $matches)) {
if ($this->context->getMethod() != 'PUT') {
if ($requestMethod != 'PUT') {
$allow[] = 'PUT';
goto not_bazbaz6;
}
Expand Down Expand Up @@ -195,7 +197,7 @@ public function match($pathinfo)

}

$host = $this->context->getHost();
$host = $context->getHost();

if (preg_match('#^a\\.example\\.com$#si', $host, $hostMatches)) {
// route1
Expand Down
20 changes: 11 additions & 9 deletions src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php
View file
Open in desktop
Expand Up @@ -24,8 +24,10 @@ public function match($pathinfo)
{
$allow = array();
$pathinfo = rawurldecode($pathinfo);
$trimmedPathinfo = rtrim($pathinfo, '/');
$context = $this->context;
$request = $this->request;
$requestMethod = $context->getMethod();

// foo
if (0 === strpos($pathinfo, '/foo') && preg_match('#^/foo/(?P<bar>baz|symfony)$#s', $pathinfo, $matches)) {
Expand All @@ -35,7 +37,7 @@ public function match($pathinfo)
if (0 === strpos($pathinfo, '/bar')) {
// bar
if (preg_match('#^/bar/(?P<foo>[^/]++)$#s', $pathinfo, $matches)) {
if (!in_array($this->context->getMethod(), array('GET', 'HEAD'))) {
if (!in_array($requestMethod, array('GET', 'HEAD'))) {
Copy link
Member

Choose a reason for hiding this comment

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

Reading this line makes me think that we could even have $requestMethod set to GET when it's HEAD, so that this condition can be simplified to just if ('GET' !== $requestMethod) { WDTY?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't we then loose the ability to distinguish the request when there's only a HEAD route defined? I see in the there's a case for adding HEAD to the accepted methods when it's a GET request, but not the other way around. So if we only have a HEAD registered, we wouldn't be able to match that anymore, right? This would only be the case if you want to support defining separate HEAD handlers, I don't know if that's currently the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just test this out real quick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested this out. We'd loose the ability to define HEAD routes when we make that additional optimisation.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I wasn't clear enough. My idea was to create a specific variable (different from $requestMethod, like $isLikeGetMethod or something) that holds GET when the real method is HEAD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That shaved off a 0.2ms, which is not a lot. Was done in this commit: 0425f33

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've implemented it in such a way that it keeps current behaviour, which also became more clear to me. If there's a HEAD registered after a GET for the same route, the first route will be matched. So now there is also a variable for the special "GET is also HEAD" case. The compiled route needs to remain aware whether there was actually a HEAD clause in there initially so it uses the right request method variable, the not "special" one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I begin to wonder though, the code generating this, and executing it, become less easy to understand. Since the boost is so enormously tiny of that last bit I suggest to consider reverting that one commit and keep the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the tests for the other optimisation just in case. It's 03:00 here now, so bed time. Have a good one 👍

$allow = array_merge($allow, array('GET', 'HEAD'));
goto not_bar;
}
Expand All @@ -46,7 +48,7 @@ public function match($pathinfo)

// barhead
if (0 === strpos($pathinfo, '/barhead') && preg_match('#^/barhead/(?P<foo>[^/]++)$#s', $pathinfo, $matches)) {
if (!in_array($this->context->getMethod(), array('GET', 'HEAD'))) {
if (!in_array($requestMethod, array('GET', 'HEAD'))) {
$allow = array_merge($allow, array('GET', 'HEAD'));
goto not_barhead;
}
Expand All @@ -70,7 +72,7 @@ public function match($pathinfo)
}

// baz3
if (rtrim($pathinfo, '/') === '/test/baz3') {
if ($trimmedPathinfo === '/test/baz3') {
if (substr($pathinfo, -1) !== '/') {
return $this->redirect($pathinfo.'/', 'baz3');
}
Expand All @@ -91,7 +93,7 @@ public function match($pathinfo)

// baz5
if (preg_match('#^/test/(?P<foo>[^/]++)/$#s', $pathinfo, $matches)) {
if ($this->context->getMethod() != 'POST') {
if ($requestMethod != 'POST') {
$allow[] = 'POST';
goto not_baz5;
}
Expand All @@ -102,7 +104,7 @@ public function match($pathinfo)

// baz.baz6
if (preg_match('#^/test/(?P<foo>[^/]++)/$#s', $pathinfo, $matches)) {
if ($this->context->getMethod() != 'PUT') {
if ($requestMethod != 'PUT') {
$allow[] = 'PUT';
goto not_bazbaz6;
}
Expand Down Expand Up @@ -174,7 +176,7 @@ public function match($pathinfo)
}

// hey
if (rtrim($pathinfo, '/') === '/multi/hey') {
if ($trimmedPathinfo === '/multi/hey') {
Copy link
Contributor

Choose a reason for hiding this comment

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

if ('/multi/hey' === $trimmedPathinfo) {

if (substr($pathinfo, -1) !== '/') {
return $this->redirect($pathinfo.'/', 'hey');
}
Expand Down Expand Up @@ -207,7 +209,7 @@ public function match($pathinfo)

}

$host = $this->context->getHost();
$host = $context->getHost();

if (preg_match('#^a\\.example\\.com$#si', $host, $hostMatches)) {
// route1
Expand Down Expand Up @@ -322,7 +324,7 @@ public function match($pathinfo)
// secure
if ($pathinfo === '/secure') {
$requiredSchemes = array ( 'https' => 0,);
if (!isset($requiredSchemes[$this->context->getScheme()])) {
if (!isset($requiredSchemes[$context->getScheme()])) {
return $this->redirect($pathinfo, 'secure', key($requiredSchemes));
}

Expand All @@ -332,7 +334,7 @@ public function match($pathinfo)
// nonsecure
if ($pathinfo === '/nonsecure') {
$requiredSchemes = array ( 'http' => 0,);
if (!isset($requiredSchemes[$this->context->getScheme()])) {
if (!isset($requiredSchemes[$context->getScheme()])) {
return $this->redirect($pathinfo, 'nonsecure', key($requiredSchemes));
}

Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php
View file
Open in desktop
Expand Up @@ -24,8 +24,10 @@ public function match($pathinfo)
{
$allow = array();
$pathinfo = rawurldecode($pathinfo);
$trimmedPathinfo = rtrim($pathinfo, '/');
$context = $this->context;
$request = $this->request;
$requestMethod = $context->getMethod();

if (0 === strpos($pathinfo, '/rootprefix')) {
// static
Expand Down