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.

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

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

Closed
wants to merge 12 commits into from

Conversation

@frankdejonge
Copy link
Contributor

commented Feb 24, 2017

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR does not apply

The application I'm working on is fairly large. Because we had a routing issue (not caused by the framework) I looked through the dumped routing code. I spotted some easy wins. These changes brought down the time for the match method to run from ~ 7.5ms to ~2.5ms. It's not a lot, but it's something. I've profiled it several times with blackfire to confirm. The results were very consistent. Mind you, our application has quite a serious amount of routes, a little over 900.

@@ -276,7 +278,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
} else {
$methods = implode("', '", $methods);
$code .= <<<EOF
if (!in_array(\$this->context->getMethod(), array('$methods'))) {

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 24, 2017

Author Contributor

This removes N function calls.

@@ -266,7 +268,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
if ($methods) {
if (1 === count($methods)) {
$code .= <<<EOF
if (\$this->context->getMethod() != '$methods[0]') {

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 24, 2017

Author Contributor

This removes N function calls.

@@ -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)) {
if ($supportsTrailingSlash && substr($m['url'], -1) === '/') {
$conditions[] = sprintf("rtrim(\$pathinfo, '/') === %s", var_export(rtrim(str_replace('\\', '', $m['url']), '/'), true));

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 24, 2017

Author Contributor

This removed N trim calls.

@@ -133,7 +135,7 @@ private function compileRoutes(RouteCollection $routes, $supportsRedirections)
foreach ($groups as $collection) {
if (null !== $regex = $collection->getAttribute('host_regex')) {
if (!$fetchedHost) {
$code .= " \$host = \$this->context->getHost();\n\n";

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 24, 2017

Author Contributor

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

@@ -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'))) {

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 25, 2017

Member

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?

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 25, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 25, 2017

Author Contributor

I'll just test this out real quick.

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 25, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 25, 2017

Member

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.

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 25, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 25, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 25, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 25, 2017

Author Contributor

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 👍

frankdejonge added some commits Feb 25, 2017

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Feb 25, 2017

$context = $this->context;
$request = $this->request;
$requestMethod = $isLikeGetMethod = $context->getMethod();
if ($requestMethod === 'HEAD') {

This comment has been minimized.

Copy link
@hhamon

hhamon Feb 27, 2017

Contributor

Should be if ('HEAD' === $requestMethod) {

@@ -46,8 +53,8 @@ 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'))) {
$allow = array_merge($allow, array('GET', 'HEAD'));
if ($isLikeGetMethod != 'GET') {

This comment has been minimized.

Copy link
@hhamon

hhamon Feb 27, 2017

Contributor

if ('GET' !== $isLikeGetMethod) {

@@ -83,7 +90,7 @@ public function match($pathinfo)
// baz5
if (preg_match('#^/test/(?P<foo>[^/]++)/$#s', $pathinfo, $matches)) {
if ($this->context->getMethod() != 'POST') {
if ($isLikeGetMethod != 'POST') {

This comment has been minimized.

Copy link
@hhamon

hhamon Feb 27, 2017

Contributor

if ('POST' !== $isLikeGetMethod) {

@@ -94,7 +101,7 @@ public function match($pathinfo)
// baz.baz6
if (preg_match('#^/test/(?P<foo>[^/]++)/$#s', $pathinfo, $matches)) {
if ($this->context->getMethod() != 'PUT') {
if ($isLikeGetMethod != 'PUT') {

This comment has been minimized.

Copy link
@hhamon

hhamon Feb 27, 2017

Contributor

if ('PUT' !== $isLikeGetMethod) {

$context = $this->context;
$request = $this->request;
$requestMethod = $isLikeGetMethod = $context->getMethod();
if ($requestMethod === 'HEAD') {

This comment has been minimized.

Copy link
@hhamon

hhamon Feb 27, 2017

Contributor

if ('HEAD' === $isLikeGetMethod) {

@@ -46,8 +53,8 @@ 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'))) {
$allow = array_merge($allow, array('GET', 'HEAD'));
if ($isLikeGetMethod != 'GET') {

This comment has been minimized.

Copy link
@hhamon

hhamon Feb 27, 2017

Contributor

if ('GET' !== $isLikeGetMethod) {

@@ -91,7 +98,7 @@ public function match($pathinfo)
// baz5
if (preg_match('#^/test/(?P<foo>[^/]++)/$#s', $pathinfo, $matches)) {
if ($this->context->getMethod() != 'POST') {
if ($isLikeGetMethod != 'POST') {

This comment has been minimized.

Copy link
@hhamon

hhamon Feb 27, 2017

Contributor

if ('POST' !== $isLikeGetMethod) {

@@ -102,7 +109,7 @@ public function match($pathinfo)
// baz.baz6
if (preg_match('#^/test/(?P<foo>[^/]++)/$#s', $pathinfo, $matches)) {
if ($this->context->getMethod() != 'PUT') {
if ($isLikeGetMethod != 'PUT') {

This comment has been minimized.

Copy link
@hhamon

hhamon Feb 27, 2017

Contributor

if ('PUT' !== $isLikeGetMethod) {

@frankdejonge

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

@hhamon I get it, yoda-ing it now :P

@@ -174,7 +181,7 @@ public function match($pathinfo)
}
// hey
if (rtrim($pathinfo, '/') === '/multi/hey') {
if ($trimmedPathinfo === '/multi/hey') {

This comment has been minimized.

Copy link
@hhamon

hhamon Feb 27, 2017

Contributor

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

$context = $this->context;
$request = $this->request;
$requestMethod = $isLikeGetMethod = $context->getMethod();
if ($requestMethod === 'HEAD') {

This comment has been minimized.

Copy link
@hhamon

hhamon Feb 27, 2017

Contributor

if ('HEAD' === $requestMethod) {

@frankdejonge

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

@hhamon I've corrected the non-yoda to yoda statement, I've also corrected the ones which I didn't change into yoda statements.

\$requestMethod = \$isLikeGetMethod = \$context->getMethod();
if (\$requestMethod === 'HEAD') {
\$isLikeGetMethod = 'GET';

This comment has been minimized.

Copy link
@stof

stof Feb 27, 2017

Member

the $isLikeGetMethod variable name makes me think it is a boolean

if (!in_array($this->context->getMethod(), array('GET', 'HEAD'))) {
$allow = array_merge($allow, array('GET', 'HEAD'));
if (!in_array($isLikeGetMethod, array('GET'))) {
$allow = array_merge($allow, array('GET'));

This comment has been minimized.

Copy link
@stof

stof Feb 27, 2017

Member

not adding HEAD anymore to the allow array looks suspicious to me.

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 27, 2017

Author Contributor

@stof I'm open to suggestions here. The name was given by @fabpot initially, I couldn't come up with a better name for it now. The name goes into the realm of "normalised" pretty quickly which doesn't add a lot of context.

This comment has been minimized.

Copy link
@stof

stof Feb 27, 2017

Member

@frankdejonge are you sure you replied to the right comment ? This looks unrelated

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 27, 2017

Author Contributor

@stof I think github must have durped, because I looked at it when writing but now it's placed here. Super weird.

frankdejonge added some commits Feb 27, 2017

CS
@frankdejonge

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

@stof I've corrected the case where only one method is present after filtering out the HEAD method.

if (!in_array($this->context->getMethod(), array('GET', 'HEAD'))) {
$allow = array_merge($allow, array('GET', 'HEAD'));
if ('GET' !== $isLikeGetMethod) {
$allow[] = 'GET';

This comment has been minimized.

Copy link
@stof

stof Feb 27, 2017

Member

Why aren't you adding HEAD anymore here ? It is an allowed method

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 27, 2017

Author Contributor

@stof this was an optimisation proposed by @fabpot. We stream HEAD in a special way (this is current behaviour). When a HEAD request is made, we also match GET. Inverted, when we declare GET actions, we also match in HEAD. So in this normalised case we know that if something is "like get" we don't need HEAD anymore because we're we already match GET.

This comment has been minimized.

Copy link
@Jean85

Jean85 Feb 28, 2017

Contributor

Can we make sure that this is properly tested? If someone else breaks this, it will be a huge bug!

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 28, 2017

Author Contributor

@Jean85 there's an additional test case added in this PR to do just that.

frankdejonge added some commits Feb 27, 2017

Added a test case which more clearly demonstrates the effect of the h…
…ttp method optimization, especially the HEAD case.
@frankdejonge

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

Perhaps it would be good to rename isGetLikeMethod to methodWhereHeadMatchesGet? Might be a bit verbose, but in this case I think being explicit is preferable. WDYT @stof?

@frankdejonge

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

I've also added another test fixture which more clearly demonstrates the effect of the method optimisation.

@frankdejonge frankdejonge changed the title Optimised dumped router matcher, prevent unneeded function calls. [Routing] Optimised dumped router matcher, prevent unneeded function calls. Feb 27, 2017

\$requestMethod = \$isLikeGetMethod = \$context->getMethod();
\$schema = \$context->getScheme();
if (\$requestMethod === 'HEAD') {

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Feb 28, 2017

Contributor

'HEAD' === 😉

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 28, 2017

Author Contributor

Fixed, thanks!

\$schema = \$context->getScheme();
if ('HEAD' === \$requestMethod) {
\$isLikeGetMethod = 'GET';

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Feb 28, 2017

Member

The is prefix in this variable name looks wrong. Maybe it's too late, but what if we rename $isLikeGetMethod = 'GET' to $isSafeMethod = true

We had some discussions about what a "safe method" is in HTTP, but the section 9.1.1 of RFC 2616 mentions GET and HEAD as safe methods: http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html


Or at least, rename $isLikeGetMethod = 'GET' to $isGetOrHead = true

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 28, 2017

Author Contributor

Actually, we don't want a boolean, because then we can't perform a meaningful optimisation. We just want to have a interpreted method, which we can check against. Otherwise we'd need to do more check than less.

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Feb 28, 2017

Member

What about renaming it to $methodAlias or $canonicalMethod or something like that? We "can't" store a string in a $is* variable.

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 28, 2017

Author Contributor

I like conicalMethod, a lot. I had previously opted to use methodWhereHeadMatchesGet instead. Which is a little verbose, but it does make it clear what's going on.

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 28, 2017

Member

+1 for $canonicalMethod

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 28, 2017

Author Contributor

I've renamed the variable to $canonicalMethod.

$code .= <<<EOF
if ('$methods[0]' !== \$$methodVariable) {
\$allow[] = '$methods[0]';
goto $gotoname;

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Feb 28, 2017

Contributor

Has the use of the goto ever been discussed?
It seems like here we only need to reverse the conditions to get ride of it.

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 28, 2017

Author Contributor

I'm checking this out now, if proven to be effective I'll create another PR.

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 28, 2017

Author Contributor

@GuilhemN I suspected it to have some effect, but it did literally nothing.

@Jean85

Jean85 approved these changes Feb 28, 2017

@frankdejonge

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2017

Tests currently seem to be failing because master is broken.

\$context = \$this->context;
\$request = \$this->request;
\$requestMethod = \$canonicalMethod = \$context->getMethod();
\$schema = \$context->getScheme();

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 28, 2017

Member

should be $scheme here, not $schema, right?

This comment has been minimized.

Copy link
@frankdejonge

frankdejonge Feb 28, 2017

Author Contributor

@fabpot correct, I've fixed it.

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

👍

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

Thank you @frankdejonge.

@fabpot fabpot closed this Feb 28, 2017

fabpot added a commit that referenced this pull request Feb 28, 2017

feature #21755 [Routing] Optimised dumped router matcher, prevent unn…
…eeded function calls. (frankdejonge)

This PR was squashed before being merged into the 3.3-dev branch (closes #21755).

Discussion
----------

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

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

The application I'm working on is fairly large. Because we had a routing issue (not caused by the framework) I looked through the dumped routing code. I spotted some easy wins. These changes brought down the time for the `match` method to run from ~ 7.5ms to ~2.5ms. It's not a lot, but it's something. I've profiled it several times with blackfire to confirm. The results were very consistent. Mind you, our application has quite a serious amount of routes, a little over 900.

Commits
-------

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

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.