Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Routing] Optimised dumped router matcher, prevent unneeded function calls.#21755
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 ourterms of service andprivacy 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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| }else { | ||
| $methods =implode("', '",$methods); | ||
| $code .=<<<EOF | ||
| if (!in_array(\$this->context->getMethod(), array('$methods'))) { |
There was a problem hiding this comment.
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.
| if ($methods) { | ||
| if (1 ===count($methods)) { | ||
| $code .=<<<EOF | ||
| if (\$this->context->getMethod() != '$methods[0]') { |
There was a problem hiding this comment.
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.
| 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)); |
There was a problem hiding this comment.
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.
| foreach ($groupsas$collection) { | ||
| if (null !==$regex =$collection->getAttribute('host_regex')) { | ||
| if (!$fetchedHost) { | ||
| $code .="\$host =\$this->context->getHost();\n\n"; |
There was a problem hiding this comment.
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?
| // 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'))) { |
There was a problem hiding this comment.
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 toGET when it'sHEAD, so that this condition can be simplified to justif ('GET' !== $requestMethod) { WDTY?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 holdsGET when the real method isHEAD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That shaved off a0.2ms, which is not a lot. Was done in this commit:0425f33
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
| $request =$this->request; | ||
| $requestMethod =$isLikeGetMethod =$context->getMethod(); | ||
| if ($requestMethod ==='HEAD') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should beif ('HEAD' === $requestMethod) {
| 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') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if ('GET' !== $isLikeGetMethod) {
| // baz5 | ||
| if (preg_match('#^/test/(?P<foo>[^/]++)/$#s',$pathinfo,$matches)) { | ||
| if ($this->context->getMethod() !='POST') { | ||
| if ($isLikeGetMethod !='POST') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if ('POST' !== $isLikeGetMethod) {
| // baz.baz6 | ||
| if (preg_match('#^/test/(?P<foo>[^/]++)/$#s',$pathinfo,$matches)) { | ||
| if ($this->context->getMethod() !='PUT') { | ||
| if ($isLikeGetMethod !='PUT') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if ('PUT' !== $isLikeGetMethod) {
| $request =$this->request; | ||
| $requestMethod =$isLikeGetMethod =$context->getMethod(); | ||
| if ($requestMethod ==='HEAD') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if ('HEAD' === $isLikeGetMethod) {
| 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') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if ('GET' !== $isLikeGetMethod) {
| // baz5 | ||
| if (preg_match('#^/test/(?P<foo>[^/]++)/$#s',$pathinfo,$matches)) { | ||
| if ($this->context->getMethod() !='POST') { | ||
| if ($isLikeGetMethod !='POST') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if ('POST' !== $isLikeGetMethod) {
| // baz.baz6 | ||
| if (preg_match('#^/test/(?P<foo>[^/]++)/$#s',$pathinfo,$matches)) { | ||
| if ($this->context->getMethod() !='PUT') { | ||
| if ($isLikeGetMethod !='PUT') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if ('PUT' !== $isLikeGetMethod) {
frankdejonge commentedFeb 27, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@hhamon I get it, yoda-ing it now :P |
| // hey | ||
| if (rtrim($pathinfo,'/') ==='/multi/hey') { | ||
| if ($trimmedPathinfo ==='/multi/hey') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if ('/multi/hey' === $trimmedPathinfo) {
| $request =$this->request; | ||
| $requestMethod =$isLikeGetMethod =$context->getMethod(); | ||
| if ($requestMethod ==='HEAD') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if ('HEAD' === $requestMethod) {
frankdejonge commentedFeb 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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not adding HEAD anymore to the allow array looks suspicious to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@frankdejonge are you sure you replied to the right comment ? This looks unrelated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@stof I think github must have durped, because I looked at it when writing but now it's placed here. Super weird.
frankdejonge commentedFeb 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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why aren't you adding HEAD anymore here ? It is an allowed method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we make sure that this is properly tested? If someone else breaks this, it will be a huge bug!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Jean85 there's an additional test case added in this PR to do just that.
…ttp method optimization, especially the HEAD case.
frankdejonge commentedFeb 27, 2017
Perhaps it would be good to rename |
frankdejonge commentedFeb 27, 2017
I've also added another test fixture which more clearly demonstrates the effect of the method optimisation. |
| \$requestMethod =\$isLikeGetMethod =\$context->getMethod(); | ||
| \$schema =\$context->getScheme(); | ||
| if (\$requestMethod === 'HEAD') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
'HEAD' === 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed, thanks!
| \$schema =\$context->getScheme(); | ||
| if ('HEAD' ===\$requestMethod) { | ||
| \$isLikeGetMethod = 'GET'; |
javiereguiluzFeb 28, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Theis 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 mentionsGET andHEAD as safe methods:http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html
Or at least, rename$isLikeGetMethod = 'GET' to$isGetOrHead = true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about renaming it to$methodAlias or$canonicalMethod or something like that? We "can't" store a string in a$is* variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I likeconicalMethod, a lot. I had previously opted to usemethodWhereHeadMatchesGet instead. Which is a little verbose, but it does make it clear what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
+1 for$canonicalMethod
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've renamed the variable to$canonicalMethod.
| $code .=<<<EOF | ||
| if ('$methods[0]' !==\$$methodVariable) { | ||
| \$allow[] = '$methods[0]'; | ||
| goto$gotoname; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm checking this out now, if proven to be effective I'll create another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@GuilhemN I suspected it to have some effect, but it did literally nothing.
frankdejonge commentedFeb 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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be$scheme here, not$schema, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@fabpot correct, I've fixed it.
fabpot commentedFeb 28, 2017
👍 |
fabpot commentedFeb 28, 2017
Thank you@frankdejonge. |
…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 applyThe 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.
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
matchmethod 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.