Performance increasement by integer priority #17
Conversation
|
What about the registering events stuff? You increased th executed function count by quite a lot |
|
The general logic for registering events stuff (attach / detach) hasn't changed.
I'll create some benchmarks affecting this and a benchmark with the event structure of the skeleton application as real world scenario. (Or do you have a better real world application where I can get the event structure from?) Including PHP 7 bench as I think the overhead of a build-in function call has been improved a lot. Marc |
| } | ||
| if ($this->sharedManager) { | ||
| foreach ($this->sharedManager->getListeners($this->identifiers, $eventName) as $p => $l) { | ||
| $listeners[$p] = isset($listeners[$p]) ? array_merge($listeners[$p], $l) : $l; |
There was a problem hiding this comment.
array_merge() is called repeatedly now
|
@marc-mabe my main concern is at https://github.com/zendframework/zend-eventmanager/pull/17/files#r47010631 |
|
Yes it is but only for wildcard and shared events if there are already events attached with the same priority. |
|
@Ocramius I restructured the internal array of listeners generated. This looks a bit strange but it no longer merges the list of listeners into one list but instead create a new small list per list where the existing lists get pushed to. This way it reduces the internal iterations of each list and also reduces the amount of memory allocated for the merged list which improves performance ;) I created an own benchmark script that shows a big speed-up bench_em.phpAthletic |
| @@ -27,7 +27,7 @@ public function setUp() | |||
|
|
|||
| public function getListeners(SharedEventManager $manager, array $identifiers, $event, $priority = 1) | |||
| { | |||
| $priority = (int) $priority . '.0'; | |||
| $priority = (int) $priority; | |||
There was a problem hiding this comment.
Note to self: this will affect every single component we've upgraded that implements a "getListeners" method of some sort for introspecting attached listeners.
There was a problem hiding this comment.
Which may be a rationale for re-instating the getListeners() method. That said, it could be marked private, and libraries could use reflection in order to query it.
This is me brainstorming via github comments. Sue me.
|
@marc-mabe one last thing for this PR: can we move the benchmarks into athletic benchmarks (as part of the zf2 suite)? @weierophinney do we have any tool to store the results over time, so that we don't degrade performance later on? |
|
@Ocramius I don't know if exists a tool to manage and check benchmark, it is a requirements for zend framework?
|
|
@Ocramius we don't currently have a way to store the results; at this time, I've been looking through past PRs that do performance changes to see what happens. We could potentially cache the results on Travis; the problem will be writing a script that does a diff between the cache and the current run. |
|
@weierophinney that seems to be a bit fragile - travis clears the files and boom. What about pushing the states to a secondary repo via an encrypted SSH key? |
|
@Ocramius @weierophinney It would be great to have a benchmark monitoring system but it would be required to run the same benchmark in exactly the same environment (same HW resources + same software + no concurrent running jobs) on different software versions (last commit hashes and properly last release). This makes it hard running benchmarks automatically and monitor them. And Travis doesn't match this requirements. |
|
@Ocramius Sure, I can add this small bench script to athletic but there are already benchmarks for it - the main reason for writing my own was compatibility with PHP-7.0 and to have a better comparison between both versions incl. memory consumption. |
|
@marc-mabe the performance tracking discussion is something the @phpbench folks are working on via phpbench/phpbench#252. |
|
@marc-mabe Does this supercede #16? |
|
@marc-mabe Nevermind, answered my own question; it does. |
|
I've just performed benchmarks across each of PHP 5.5, 5.6, and 7, on both the current develop branch and a version merging this patch. Here's what I'm seeing, after re-running the benchmarks several times to see general trends:
All others (typically) trend faster with the patch applied. I'm really not sure what to do here. While the general trend is faster, the specific metric I'm most interested in is the one listed above, as that's the one in play when using zend-mvc, and thus the metric that will most affect users. |
|
By how much percent all the other uses cases are faster? I suppose the MultipleEventMultipleSharedListener is slower because of the multiple foreach instead of array_merge_recursive, right? |
|
@bakura10 I didn't profile to see exactly where the slowdown happens, only which scenario is slower. :) The other scenarios were typically in the 5% faster range, with some as high as 15-20%. |
|
@weierophinney |
|
Me too. From @marc-mabe it seems the increase in all other scenarios is subtanstial enough to cope this highly specific use case. |
|
@weierophinney does the MVC have multiple shared listeners? O_o |
|
@Ocramius Typically, yes. Most modules I've reviewed will:
We even do so in quite a number of Apigility modules, and the above has been a recommended best practice for module development made in our tutorials, in the training courses, and even the certification exam. |
|
Let me try if I can improve this case to get at least the same performance as before. |
|
@weierophinney @Ocramius ping |
|
Moved to 3.1.0 |
| // If the event was asked to stop propagating, do so | ||
| if ($event->propagationIsStopped()) { | ||
| $responses->setStopped(true); | ||
| break 3; |
There was a problem hiding this comment.
This is basically return $responses, no?
|
@marc-mabe a couple more things (sorry in advance):
|
|
@Ocramius thanks for reviewing this! |
Not sure either. If it has a visible impact, then I guess it's fine as-is, but the complexity of the code is indeed huge already... |
|
@Ocramius I have renamed all variables and improved coding style a little. Also did one or two micro optimizations committed to this branch :) BenchmarksPHP 5.6.24-0+deb8u1
PHP 7.0.9
|
|
@Ocramius @weierophinney Can this PR be merged or still is something missing? |
|
Looks good to me, but the code is indeed harder to read. @weierophinney I think this is OK to merge, given the improvements. |
After my PR #16 I experimented with integer priorities.
It's no longer storing priorities as string with
.0suffix but store as integer. This makes it harder to merge listeners of the same priority by name, wildcard or shared but in the end I sow a very good performance improvement, onlyMultipleEventMultipleSharedListenerdecreases a little (see below).All tests passed for me but it should be tested carefully anyway ;)
PHP 5.6
PHP 5.6.14-0+deb8u1 (cli) (built: Oct 4 2015 16:13:10)
Copyright (c) 1997-2015 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2015 Zend Technologies
with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2015, by Zend Technologies
Branch: develop
Branch: integer_priority
PHP 7.0
PHP 7.0.0 (cli) (built: Dec 5 2015 23:50:11) ( NTS )
Copyright (c) 1997-2015 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2015 Zend Technologies
Something with athletic doesn't work with PHP-7 but I have no time to figure that out.