[EventDispatcher] small optimization #19312

cheprasov · 2016-07-07T23:01:31Z

Q	A
Branch?	2.7
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

$this->listenerIds || $this->listeners returns bool
isset($params[0]) is faster and safer than is_array($params)

fabpot · 2016-07-08T06:48:20Z

We don't make "performance optimization" without proofs that it indeed improves performance significantly. Can you tell us what kind of improvement you see with your changes?

cheprasov · 2016-07-08T06:58:44Z

@fabpot
also isset($params[0]) is safe function for is_array($params[0])

Because is_array($params) && is_array($params[0]) if $params[0] is not setted, you will get Notice.

And I did test yesterday

+-------+------------+-------+------------------+---------------------+-------+
| GROUP | NAME       | COUNT | TIME             | SINGLE              | COST  |
+-------+------------+-------+------------------+---------------------+-------+
| Test  | is_array() | 1000  | 0.16683268547058 | 0.00016683268547058 | 152 % |
| Test  | isset()    | 1000  | 0.10995626449585 | 0.00010995626449585 | 100 % |
+-------+------------+-------+------------------+---------------------+-------+

upd.

+-------+--------------------------------------+-------+------------------+---------------------+-------+
| GROUP | NAME                                 | COUNT | TIME             | SINGLE              | COST  |
+-------+--------------------------------------+-------+------------------+---------------------+-------+
| Test  | (bool) count($a) || (bool) count($b) | 1000  | 0.41624760627747 | 0.00041624760627747 | 126 % |
| Test  | count($a) || count($b)               | 1000  | 0.40643239021301 | 0.00040643239021301 | 123 % |
| Test  | $a || $b                             | 1000  | 0.32996344566345 | 0.00032996344566345 | 100 % |
+-------+--------------------------------------+-------+------------------+---------------------+-------+

eugene-matvejev · 2016-07-08T07:01:43Z

previously I seen that guys submit https://blackfire.io/ metrics with PR, may be that will solve problem?

chalasr · 2016-07-08T21:32:55Z

src/Symfony/Component/EventDispatcher/ContainerAwareEventDispatcher.php

@@ -105,7 +105,7 @@ public function removeListener($eventName, $listener)
    public function hasListeners($eventName = null)
    {
        if (null === $eventName) {
-            return (bool) count($this->listenerIds) || (bool) count($this->listeners);
+            return count($this->listenerIds) || count($this->listeners);


what about return $this->listenerIds || $this->listeners?

Thanks. Fixed.
Result of performance test.

+-------+------------------------+-------+------------------+---------------------+-------+ | GROUP | NAME | COUNT | TIME | SINGLE | COST | +-------+------------------------+-------+------------------+---------------------+-------+ | Test | count($a) || count($b) | 1000 | 0.40582633018494 | 0.00040582633018494 | 123 % | | Test | $a || $b | 1000 | 0.32871198654175 | 0.00032871198654175 | 100 % | +-------+------------------------+-------+------------------+---------------------+-------+

nicolas-grekas · 2016-07-10T07:13:28Z

👍 (with --switch=2.7 when merging)

xabbuh · 2016-07-10T07:36:36Z

src/Symfony/Component/EventDispatcher/ContainerAwareEventDispatcher.php

@@ -105,7 +105,7 @@ public function removeListener($eventName, $listener)
    public function hasListeners($eventName = null)
    {
        if (null === $eventName) {
-            return (bool) count($this->listenerIds) || (bool) count($this->listeners);
+            return $this->listenerIds || $this->listeners;


In #19320 we rejected to replace count() with empty(). Imo we should be consistent with our decisions here.

Indeed. I think the current expression better conveys the intent.

#19320 was a broad PR changing everything at once. On a case by case basis, we not to me.
I this case, casting to bool feels strange. At least, I'd expect 0 < count(...). But personally (and I know I'm not the usual guy concerning CS), I prefer the proposed way. Less steps in my personal PHP virtual machine :)

Changing this line to return 0 < count($this->listenerIds) || 0 < count($this->listeners); would be okay for me.

Why we need to use return 0 < count($this->listenerIds) || 0 < count($this->listeners) ?
Do you want be sure, that you will get bool?
But, if I use logical operators (||, &&, and so on) I will get bool.
For example,
return 10 || 0; you will not get 10, you will get result of logical operation, it will be true, we do not need to convert it to bool.

I think debating this is pointless. Let's keep what we already have as it works and let's move on.

Maybe is better to write clear and optimized code and use comments and annotations for easy understanding?

I think, some constructions like as return 0 < count($this->listenerIds) || 0 < count($this->listeners) is not easier to understanding, or it maybe easier, for people, who do not know php enough well.

I believe, to use code constructions that have excess of logic is not correct way to build great and fast framework.

I am not Symfony person, and I can't decide, but whenever you attend LondonPHP meetups, good speakers advertise to write clean code [and I agree with them], which I think this PR aiming for, if it get us tiny performance improvement I think we should consider to adopt it :)

Please, define "clean code" in this context.

As someone not familiar with this piece of code, return 0 < count($this->listenerIds) || 0 < count($this->listeners); looks much easier to understand than return (bool) count($this->listenerIds) || (bool) count($this->listeners) and return $this->listenerIds || $this->listeners;.

EventDispatcher - small optimization

015e2c7

carsonbot added Status: Needs Review EventDispatcher labels Jul 7, 2016

chalasr reviewed Jul 8, 2016
View reviewed changes

EventDispatcher - small optimization

11652cb

xabbuh reviewed Jul 10, 2016
View reviewed changes

cheprasov closed this Jul 16, 2016

chalasr mentioned this pull request Aug 30, 2016

[Console] Only show namespaces with commands #19776

Closed

Uh oh!

[EventDispatcher] small optimization #19312

[EventDispatcher] small optimization #19312

Uh oh!

Conversation

cheprasov commented Jul 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabpot commented Jul 8, 2016

Uh oh!

cheprasov commented Jul 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eugene-matvejev commented Jul 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chalasr Jul 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jul 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheprasov Jul 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eugene-matvejev Jul 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theofidry Jul 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cheprasov commented Jul 7, 2016 •

edited

Loading

cheprasov commented Jul 8, 2016 •

edited

Loading

eugene-matvejev commented Jul 8, 2016 •

edited

Loading

chalasr Jul 8, 2016 •

edited

Loading

nicolas-grekas commented Jul 10, 2016 •

edited

Loading

cheprasov Jul 13, 2016 •

edited

Loading

eugene-matvejev Jul 13, 2016 •

edited

Loading

theofidry Jul 15, 2016 •

edited

Loading