diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

merged branch pulzarraider/explode_optimalisation (PR #2782)

Browse Source 
Commits
-------

cd24fb8 change explode's limit parameter based on known variable content
b3cc270 minor optimalisations for explode

Discussion
----------

[FrameworkBundle][CssSelector][HttpFoundation][HttpKernel] [Security][Validator] Minor optimizations for "explode" function

Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -

I added limit parameter in some places, where it may be usefull. I did not check the context of what values may have been exploded. So to not break anything, I added +1 to limit parameter.

If you find out that in some places limit (or limit+1) is not important or meaningless, write a comment please and I will fix it.

---------------------------------------------------------------------------

by fabpot at 2011/12/07 06:56:49 -0800

Adding +1 just to be sure to not break anything is clearly something we won't do. What is the benefit of doing that anyway?

---------------------------------------------------------------------------

by pulzarraider at 2011/12/07 13:50:24 -0800

The main idea of making this PR was to notify about some places that may run faster with just adding one parameter to explode function.

If in code is someting like: ```list($a, $b) = explode(':', $s);```
Function ```explode``` will create n-items (depends on ```$s```), but we need in code only the first two items. There is no reason to let ```explode``` create more items in memory that are NEVER used in our code. The limit parameter is there for these situations, so let's use it.

I know that it is microoptimization and may look unimportant, but we are writing a framework - so people expect that code will be as fast as possible without this kind of mistakes.

As I've noticed above, I know that +1 is not ideal solution, but the fastest without debugging the code. I expect that someone (with good knowledge of that code) will look at it and write in comments if variable may contain 1 comma (dot or someting on what is doing the explode) or maybe 2 in some situations or more.

Anyway, +1 will not break anything, because same items are created as it is now, but no unnecessary item is created.

---------------------------------------------------------------------------

by fabpot at 2011/12/07 23:14:59 -0800

I'm +1 for adding the number to avoid problems but I'm -1 on the optimization side of things as it won't optimize anything.

---------------------------------------------------------------------------

by helmer at 2011/12/08 12:46:49 -0800

*.. The main idea of making this PR was to notify about some places that **may** run faster ..*

I am also unsure the optimization is really an optimization, care to benchmark (with meaningful inputs)? As for the limit+1 thing, why would you want to +1 it? The number of ``list`` arguments should always reflect the ``limit`` parameter, no?

---------------------------------------------------------------------------

by pulzarraider at 2011/12/08 23:11:34 -0800

@helmer please try this simple benchmark:

```
<?php

header('Content-Type: text/plain; charset=UTF-8');
define('COUNT', 10000);

$source_string = 'aaaaaaaaaaaaaaaaaaaa:bbbbbbbbbbbbbbbbbbbbb:cccccccccccccccccccccccc:dddddddddddddddddddddd:eeeeeeeeeeeeeeeeeeeeeeeee:fffffffffffffffffffffffffff';

$start = microtime(true);
for ($i = 0; $i < COUNT; $i++) {
    list($a, $b) = explode(':', $source_string);
}
$end = microtime(true)-$start;
echo 'without limit: '.$end."\n";

$start = microtime(true);
for ($i = 0; $i < COUNT; $i++) {
    list($a, $b) = explode(':', $source_string, 2);
}
$end = microtime(true)-$start;
echo 'with limit:    '.$end."\n";
```

My results are:

```
without limit: 0.057228803634644
with limit:    0.028676986694336
```
That is 50% difference (with APC enabled).  Of course the result depends on the length of source string and if it's too short, the difference may be none or very very small. That's why I said, that it **may** run faster and is just a micro optimization.

---------------------------------------------------------------------------

by pulzarraider at 2011/12/08 23:18:12 -0800

@helmer And why +1? It depends on a code:

```
$source_string = 'aaaaaaaaaaaaaaaaaaaa:bbbbbbbbbbbbbbbbbbbbb:cccccccccccccccccccccccc';
list($a, $b) = explode(':', $source_string, 2);
var_dump($a, $b);
```

and

```
$source_string = 'aaaaaaaaaaaaaaaaaaaa:bbbbbbbbbbbbbbbbbbbbb:cccccccccccccccccccccccc';
list($a, $b) = explode(':', $source_string, 3);
var_dump($a, $b);
```
gives different results. That's why the content of the variable must be known.

---------------------------------------------------------------------------

by helmer at 2011/12/09 00:08:28 -0800

@pulzarraider Thanks for the benchmark, seems like a gain enough. Although, we are more likely having a scenario of:
``explode(':', 'a🅱️c')`` vs ``explode(':', 'a🅱️c', 3)`` with a ``COUNT`` of 10, where the difference is not even in microseconds anymore :)

The limit addition alters the behaviour though, ie suddenly you can define a controller [logical name](http://symfony.com/doc/current/book/routing.html#controller-string-syntax) as ´´AcmeBlogBundle:Blog:show:something``, and things go downhill from there on.

All that aside, I'm +1 for setting the limit to the exact number of ``list`` parameters, but certainly not number+1, this is just too wtfy (as you said, this was a safety thing, but I reckon for this PR to be merged it needs to be +0).

---------------------------------------------------------------------------

by drak at 2011/12/09 08:28:58 -0800

Overall `list()` is ugly as it's not very explicit.  Even though it would mean extra lines, it's better to `explode()` then explicitly assign variables:

```
$parts = explode(':', $foo);
$name = $parts[0];
$tel = $parts[1];
```

`list()` is one of those bad relics from the PHP past...

---------------------------------------------------------------------------

by fabpot at 2011/12/11 10:07:47 -0800

@drak: why is `list` not explicit? It is in fact as explicit as the more verbose syntax you propose.

---------------------------------------------------------------------------

by pulzarraider at 2011/12/11 13:08:50 -0800

@drak: I agree with @fabpot. In speech of benchmarks ```list``` is faster then using a helper variable.

@fabpot, @helmer I've changed explode's limit to be correct (without +1) and removed some changes from this PR, where I can't find out what the content of variable may be. Unit tests pass, so I think it's ready for merge.
This commit is contained in:
Fabien Potencier 2011-12-13 17:39:32 +01:00
commit 12ea7568a0
7 changed files with 10 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/Symfony/Bundle/FrameworkBundle/Controller/ControllerResolver.php
View File

@ -58,7 +58,7 @@ class ControllerResolver extends BaseControllerResolver
$controller = $this->parser->parse($controller);
} elseif (1 == $count) {
// controller in the service:method notation
list($service, $method) = explode(':', $controller);
list($service, $method) = explode(':', $controller, 2);
return array($this->container->get($service), $method);
} else {
@ -66,7 +66,7 @@ class ControllerResolver extends BaseControllerResolver
}
}
list($class, $method) = explode('::', $controller);
list($class, $method) = explode('::', $controller, 2);
if (!class_exists($class)) {
throw new \InvalidArgumentException(sprintf('Class "%s" does not exist.', $class));

4
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
View File

@ -195,7 +195,7 @@ class FrameworkExtension extends Extension
'sqlite' => 'Symfony\Component\HttpKernel\Profiler\SqliteProfilerStorage',
'mysql' => 'Symfony\Component\HttpKernel\Profiler\MysqlProfilerStorage',
);
list($class, ) = explode(':', $config['dsn']);
list($class, ) = explode(':', $config['dsn'], 2);
if (!isset($supported[$class])) {
throw new \LogicException(sprintf('Driver "%s" is not supported for the profiler.', $class));
}
@ -513,7 +513,7 @@ class FrameworkExtension extends Extension
})->in($dirs);
foreach ($finder as $file) {
// filename is domain.locale.format
list($domain, $locale, $format) = explode('.', $file->getBasename());
list($domain, $locale, $format) = explode('.', $file->getBasename(), 3);
$translator->addMethodCall('addResource', array($format, (string) $file, $locale, $domain));
}

2
src/Symfony/Bundle/FrameworkBundle/Templating/Helper/CodeHelper.php
View File

@ -86,7 +86,7 @@ class CodeHelper extends Helper
public function abbrMethod($method)
{
if (false !== strpos($method, '::')) {
list($class, $method) = explode('::', $method);
list($class, $method) = explode('::', $method, 2);
$result = sprintf("%s::%s()", $this->abbrClass($class), $method);
} else if ('Closure' === $method) {
$result = sprintf("<abbr title=\"%s\">%s</abbr>", $method, $method);

2
src/Symfony/Bundle/FrameworkBundle/Tests/Templating/Helper/Fixtures/StubTemplateNameParser.php
View File

@ -28,7 +28,7 @@ class StubTemplateNameParser implements TemplateNameParserInterface
public function parse($name)
{
list($bundle, $controller, $template) = explode(':', $name);
list($bundle, $controller, $template) = explode(':', $name, 3);
if ($template[0] == '_') {
$path = $this->rootTheme.'/Custom/'.$template;

4
src/Symfony/Component/HttpFoundation/RequestMatcher.php
View File

@ -135,7 +135,7 @@ class RequestMatcher implements RequestMatcherInterface
protected function checkIp4($requestIp, $ip)
{
if (false !== strpos($ip, '/')) {
list($address, $netmask) = explode('/', $ip);
list($address, $netmask) = explode('/', $ip, 2);
if ($netmask < 1 || $netmask > 32) {
return false;
@ -158,7 +158,7 @@ class RequestMatcher implements RequestMatcherInterface
throw new \RuntimeException('Unable to check Ipv6. Check that PHP was not compiled with option "disable-ipv6".');
}
list($address, $netmask) = explode('/', $ip);
list($address, $netmask) = explode('/', $ip, 2);
$bytes_addr = unpack("n*", inet_pton($address));
$bytes_test = unpack("n*", inet_pton($requestIp));

2
src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php
View File

@ -145,7 +145,7 @@ class ControllerResolver implements ControllerResolverInterface
throw new \InvalidArgumentException(sprintf('Unable to find controller "%s".', $controller));
}
list($class, $method) = explode('::', $controller);
list($class, $method) = explode('::', $controller, 2);
if (!class_exists($class)) {
throw new \InvalidArgumentException(sprintf('Class "%s" does not exist.', $class));

2
src/Symfony/Component/Validator/Mapping/Loader/FileLoader.php
View File

@ -59,7 +59,7 @@ abstract class FileLoader implements LoaderInterface
if (strpos($name, '\\') !== false && class_exists($name)) {
$className = (string) $name;
} else if (strpos($name, ':') !== false) {
list($prefix, $className) = explode(':', $name);
list($prefix, $className) = explode(':', $name, 2);
if (!isset($this->namespaces[$prefix])) {
throw new MappingException(sprintf('Undefined namespace prefix "%s"', $prefix));