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

A few optimizations #7931

Merged
merged 3 commits into from
May 6, 2013
Merged

A few optimizations #7931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f5e7f24
[HttpFoundation] Optimize ServerBag::getHeaders()
Seldaek May 4, 2013
997d549
[HttpFoundation] Avoid a few unnecessary str_replace() calls
Seldaek May 4, 2013
ea633f5
[HttpKernel] Avoid updating the context if the request did not change
Seldaek May 4, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions src/Symfony/Component/HttpFoundation/RequestMatcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,20 +136,16 @@ 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))) {
Copy link
Member

Choose a reason for hiding this comment

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

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} ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

return false;
}
}

if (null !== $this->path) {
$path = str_replace('#', '\\#', $this->path);

if (!preg_match('#'.$path.'#', rawurldecode($request->getPathInfo()))) {
return false;
}
if (null !== $this->path && !preg_match('{'.$this->path.'}', rawurldecode($request->getPathInfo()))) {
return false;
}

if (null !== $this->host && !preg_match('#'.str_replace('#', '\\#', $this->host).'#i', $request->getHost())) {
if (null !== $this->host && !preg_match('{'.$this->host.'}i', $request->getHost())) {
return false;
}

Expand Down
5 changes: 3 additions & 2 deletions src/Symfony/Component/HttpFoundation/ServerBag.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,13 @@ class ServerBag extends ParameterBag
public function getHeaders()
{
$headers = array();
$contentHeaders = array('CONTENT_LENGTH' => true, 'CONTENT_MD5' => true, 'CONTENT_TYPE' => true);
foreach ($this->parameters as $key => $value) {
if (0 === strpos($key, 'HTTP_')) {
if ('HTTP_' === substr($key, 0, 5)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Substr is actually slower, even if the HTTP_ is not present on the string (see below), probably because strpos just iterate on the string, and substr have to allocate new memory, allocating memory is expensive. The only way substr would be faster than strpos is if string is HUGE.

php > $key = 'HTTP_asddddddddddddddddddddddddddddddssasffadssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssdassssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssfadsdddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd';
php > $start = microtime(true); for ($i = 1; $i <= 5000000; ++$i) { (0 === strpos($key, 'NOTF_')); } var_dump(microtime(true) - $start);   double(3.2372899055481)
php > $start = microtime(true); for ($i = 1; $i <= 5000000; ++$i) { ('HTTP_' === substr($key,0,5)); } var_dump(microtime(true) - $start);   double(3.6791360378265)

Copy link
Member

Choose a reason for hiding this comment

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

@fabpot Why merge it then?

Copy link
Member

Choose a reason for hiding this comment

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

Because I trust @Seldaek who spent time doing benchmarks on real-world apps.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO the rest of the PR is ok but this change should be reverted, @Seldaek what do you think ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm ok to revert this bit, but TBH either way doesn't matter so much I
guess in this case. 5mio iterations is a lot..

Copy link
Member Author

Choose a reason for hiding this comment

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

If you like to do a PR to revert that please feel free. I'm busy with
other stuff at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

done in #7943

Copy link
Contributor

Choose a reason for hiding this comment

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

@fabpot the simple benchmark was just an example. I don't need a benchmark to tell me that strpos is faster in a lot of cases than substr, I just need to think how I would implement strpos in C (is just a iteration) and i know that is fast, except in case that the string searching is huge. Example of implementation of strstr on C: ftp://ftp.pku.edu.cn/open/UCL/tcl-8.0/compat/strstr.c (strpos can be even faster the second while is not needed if just want to return the position like on strpos).

I could spent time doing benchmarks on real-world apps, and I do it but in things that require it, simple things thinking is enough.

My comment was not to raise a debate between substr vs strpos, is a micro optimization and I don't care enough about it. I commented because as @pborreli said I noticed that strpos is largely used on Symfony code base. And this pull request is called "A few optimizations" and switch strpos with substr doesn't bring any optimization.

Copy link
Contributor

Choose a reason for hiding this comment

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

@entering thanks, this change have been reverted in #7943

Copy link
Contributor

Choose a reason for hiding this comment

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

nice @pborreli

$headers[substr($key, 5)] = $value;
}
// CONTENT_* are not prefixed with HTTP_
elseif (in_array($key, array('CONTENT_LENGTH', 'CONTENT_MD5', 'CONTENT_TYPE'))) {
elseif (isset($contentHeaders[$key])) {
$headers[$key] = $value;
}
}
Expand Down
4 changes: 3 additions & 1 deletion src/Symfony/Component/HttpKernel/EventListener/RouterListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class RouterListener implements EventSubscriberInterface
private $matcher;
private $context;
private $logger;
private $request;

/**
* Constructor.
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

What if some values of the request have changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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->context->fromRequest($request);
}
$this->request = $request;
}

public function onKernelRequest(GetResponseEvent $event)
Expand Down