Skip to content

Commit

Permalink
ObjectMixin: optimized call() performance for events
Browse files Browse the repository at this point in the history
  • Loading branch information
@JanTvrdik
JanTvrdik committed Nov 10, 2014
1 parent 9959025 commit ac853ac
Showing 1 changed file with 11 additions and 5 deletions.
16 changes: 11 additions & 5 deletions src/Utils/ObjectMixin.php
View file
Expand Up @@ -49,7 +49,6 @@ public static function call($_this, $name, $args)
{
$class = get_class($_this);
$isProp = self::hasProperty($class, $name);
$methods = & self::getMethods($class);

if ($name === '') {
throw new MemberAccessException("Call to class '$class' method without name.");
Expand All @@ -66,7 +65,7 @@ public static function call($_this, $name, $args)
throw new Nette\UnexpectedValueException("Property $class::$$name must be array or NULL, " . gettype($_this->$name) ." given.");
}

} elseif (isset($methods[$name]) && is_array($methods[$name])) { // magic @methods
} elseif ($methods = & self::getMethods($class) && isset($methods[$name]) && is_array($methods[$name])) { // magic @methods
list($op, $rp, $type) = $methods[$name];
if (count($args) !== ($op === 'get' ? 0 : 1)) {
throw new Nette\InvalidArgumentException("$class::$name() expects " . ($op === 'get' ? 'no' : '1') . ' argument, ' . count($args) . ' given.');
Expand Down Expand Up @@ -226,13 +225,20 @@ private static function hasProperty($class, $name)
{
$prop = & self::$props[$class][$name];
if ($prop === NULL) {
$prop = FALSE;
try {
$rp = new \ReflectionProperty($class, $name);
if ($rp->isPublic() && !$rp->isStatic()) {
$prop = preg_match('#^on[A-Z]#', $name) ? 'event' : TRUE;
if (strncmp($name, 'on', 2) === 0 && isset($name[2]) && $name[2] >= 'A' && $name[2] <= 'Z') {
$prop = 'event';
} else {
$prop = TRUE;
}
} else {
$prop = FALSE;
}
} catch (\ReflectionException $e) {}
} catch (\ReflectionException $e) {
$prop = FALSE;
}
}
return $prop;
}
Expand Down

13 comments on commit ac853ac

@dg
Copy link

@dg dg commented on ac853ac Nov 10, 2014

Choose a reason for hiding this comment

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

Did you a benchmark test? preg is usually faster than calling more statements.

And why did you remove $prop = FALSE at beginning?

@JanTvrdik
Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, I've measure the impact. This is faster, at least on my laptop. The preg_match makes the entire event call (in worst-case / most-common scenario) 10 % slower. Removing the $prop = FALSE is micro optimization with barely measurable results. I can change it back.

@dg
Copy link

@dg dg commented on ac853ac Nov 10, 2014

Choose a reason for hiding this comment

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

10 % of 100 ms is nice, 10 % of 100 µs is premature optimization. How much it saves when you run one request in regular application?

@JanTvrdik
Copy link
Owner Author

Choose a reason for hiding this comment

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

Nette itself uses only few event calls. But application based on Nette can use a lot more events. I opened a thread on forum where I discuss real numbers and possible optimizations http://forum.nette.org/en/21287-possible-events-performance-optimizations

btw: and it's 20 % (100 vs. 120 ms for 10k unique event calls), I did the math wrong =)

@dg
Copy link

@dg dg commented on ac853ac Nov 10, 2014

Choose a reason for hiding this comment

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

I understand, but optimization like „removing $prop = FALSE for the price of new lines of code and less readability“ are absolutely useless.

preg_match is called once per property per request. When its performance is nearly the same as optimized performance, I vote for better readability.

You can replace $methods with $m, ObjectMixin with O, everything will make Nette faster, but it is really not good idea.

@dg
Copy link

@dg dg commented on ac853ac Nov 10, 2014

Choose a reason for hiding this comment

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

Moving $methods in condition is ok, but keep in mind operator precedence.

@JanTvrdik
Copy link
Owner Author

Choose a reason for hiding this comment

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

performance is nearly the same as optimized performance

You can argue that entire event system is fast enough (therefore the absolute speedup will always be low), but not that code with 20 % speedup is nearly the same. 20 % is a lot faster.

keep in mind operator precedence

It should be OK, otherwise I assume that tests would fail. Should I add brackets to make it more obvious?

elseif (($methods = & self::getMethods($class)) && isset($methods[$name]) && is_array($methods[$name]))

@dg
Copy link

@dg dg commented on ac853ac Nov 10, 2014

Choose a reason for hiding this comment

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

Braces will make it more obvious, because it works only thanks to &.

@dg
Copy link

@dg dg commented on ac853ac Nov 10, 2014

Choose a reason for hiding this comment

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

I am unable to measure any speed-up with strncmp($name, 'on', 2) variant…

@JanTvrdik
Copy link
Owner Author

Choose a reason for hiding this comment

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

How did you measure it?

Test only for the line:

$tests = [
    'preg_match' => function ($testCount, $name) {
        for ($i = 0; $i < $testCount; $i++) {
            $prop = preg_match('#^on[A-Z]#', $name) ? 'event' : TRUE;
        }
    },
    'strncmp' => function ($testCount, $name) {
        for ($i = 0; $i < $testCount; $i++) {
            $prop = strncmp($name, 'on', 2) === 0 && isset($name[2]) && $name[2] >= 'A' && $name[2] <= 'Z' ? 'event' : TRUE;
        }
    }
];

$testCount = 100000;
$name = 'onClick';
foreach ($tests as $testName => $testCallback) {
    printf("Test %s:\n", $testName);
    $time = -microtime(TRUE);
    $testCallback($testCount, $name);
    $time += microtime(TRUE);
    printf("  %-25s%5.0f ms\n", 'X', $time * 1e3);
    printf("\n");
}

Results are 126 ms vs. 53 ms on my laptop with PHP 5.6.2 (the difference is even larger with PHP 7.0.0-dev).

@dg
Copy link

@dg dg commented on ac853ac Nov 10, 2014

Choose a reason for hiding this comment

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

It is possible to do it even faster :-)

@JanTvrdik
Copy link
Owner Author

Choose a reason for hiding this comment

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

Hm, I've tried both ord($name[2]) >> 5 === 2 and ctype_upper before sending the PR but the impact on event call was too small to be measurable. Now that I've written benchmark for this line only, I see that the ord approach is slightly faster. Or is there yet another way?

@dg
Copy link

@dg dg commented on ac853ac Nov 10, 2014

Choose a reason for hiding this comment

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

a5e3e7f

Please sign in to comment.