A few optimizations #7931
A few optimizations #7931
Conversation
isset() vs in_array makes it take half the time (1ms/req here) substr() does not have to scan the whole string so it's a wee bit faster
|
It´s nice to see that symfony contributors focus on improving performances to have a more powerfull 2.3 LTS release. I also thougth to change the ake by isset + ake in some class of the HttpFoundation or other pretty used components. What do you think about that? I would like to make benchmarks to view if it can be interesting. |
| @@ -136,20 +136,20 @@ public function matches(Request $request) | |||
| } | |||
|
|
|||
| foreach ($this->attributes as $key => $pattern) { | |||
| if (!preg_match('#'.str_replace('#', '\\#', $pattern).'#', $request->attributes->get($key))) { | |||
| if (!preg_match('{'.$pattern.'}', $request->attributes->get($key))) { | |||
There was a problem hiding this comment.
I don't think using { } as delimitor is wise when you don't control the regex entered by the user as they have a special meaning in the regex. Can they really be used as delimitor when the regex is \d{3} ?
There was a problem hiding this comment.
It is quite wise yes, especially because the character has a special meaning in regexes. That means it'll already be escaped so no need to escape it. The engine can deal with \d{3} just fine since it can count the opening/closing brackets, and skip those that are escaped. That's why I tend to always use {} as delimiters, because it doesn't add yet another char you have to escape.
Due to the BC $this->setRequest() call in the onKernelRequest method, the request is set twice every time in Symfony, and RequestContext::fromRequest takes 1ms to run here so if we can skip one call it is a win.
|
@raziel057 Don't waste your time, that's not worth it... for at least two reasons:
Now, it you can demonstrate that changing some calls to isset do not break something and it makes a typical request significantly faster (more than 1%), then go for it. But trust me, that's a big challenge ;) |
|
@fabpot I think he meant using |
| @@ -72,9 +73,10 @@ public function __construct($matcher, RequestContext $context = null, LoggerInte | |||
| */ | |||
| public function setRequest(Request $request = null) | |||
| { | |||
| if (null !== $request) { | |||
| if (null !== $request && $this->request !== $request) { | |||
There was a problem hiding this comment.
What if some values of the request have changed?
There was a problem hiding this comment.
Requests should really not be changed.. And as described in the commit, the problem is that it's always called twice for BC with frameworks that don't call setRequest
There was a problem hiding this comment.
Yes they should not change. But unfortunately requests are not immutable. So you basically can't make this assumtion. As this its a BC break.
This PR was merged into the master branch. Discussion ---------- A few optimizations Commits ------- ea633f5 [HttpKernel] Avoid updating the context if the request did not change 997d549 [HttpFoundation] Avoid a few unnecessary str_replace() calls f5e7f24 [HttpFoundation] Optimize ServerBag::getHeaders()
No description provided.