NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[EventDispatcher] small optimization#19312

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

cheprasov wants to merge2 commits intosymfony:masterfromcheprasov:event-dispatcher-small-optimization

Closed

[EventDispatcher] small optimization#19312

cheprasov wants to merge2 commits intosymfony:masterfromcheprasov:event-dispatcher-small-optimization

Conversation

Copy link

Contributor

cheprasov commentedJul 7, 2016•
edited
Loading

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 returnsbool
isset($params[0]) is faster and safer thanis_array($params)

EventDispatcher - small optimization

015e2c7

carsonbot added Status: Needs Review EventDispatcher labels

Jul 7, 2016

Copy link

Member

fabpot commentedJul 8, 2016

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?

Copy link

ContributorAuthor

cheprasov commentedJul 8, 2016•
edited
Loading

@fabpot
alsoisset($params[0]) is safe function foris_array($params[0])

Becauseis_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 % |+-------+--------------------------------------+-------+------------------+---------------------+-------+

Copy link

eugene-matvejev commentedJul 8, 2016•
edited
Loading

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

chalasr reviewed

Jul 8, 2016

View reviewed changes

src/Symfony/Component/EventDispatcher/ContainerAwareEventDispatcher.php Outdated

		{
		if (null ===$eventName) {
		return(bool)count($this->listenerIds) \|\| (bool)count($this->listeners);
		returncount($this->listenerIds) \|\|count($this->listeners);

Copy link

Member

chalasrJul 8, 2016•
edited
Loading

Choose a reason for hiding this comment

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

what aboutreturn $this->listenerIds || $this->listeners?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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 % |+-------+------------------------+-------+------------------+---------------------+-------+

EventDispatcher - small optimization

11652cb

Copy link

Member

nicolas-grekas commentedJul 10, 2016•
edited
Loading

👍 (with--switch=2.7 when merging)

xabbuh reviewed

Jul 10, 2016

View reviewed changes

src/Symfony/Component/EventDispatcher/ContainerAwareEventDispatcher.php

		{
		if (null ===$eventName) {
		return(bool)count($this->listenerIds) \|\|(bool)count($this->listeners);
		return$this->listenerIds \|\|$this->listeners;

Copy link

Member

Choose a reason for hiding this comment

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

In#19320 we rejected to replacecount() withempty(). Imo we should be consistent with our decisions here.

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

Member

Choose a reason for hiding this comment

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

#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 expect0 < 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 :)

Copy link

Member

xabbuhJul 13, 2016

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

cheprasovJul 13, 2016

Choose a reason for hiding this comment

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

Why we need to usereturn 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 betrue, we do not need to convert it to bool.

Copy link

Member

fabpotJul 13, 2016

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

cheprasovJul 13, 2016•
edited
Loading

Choose a reason for hiding this comment

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

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

I think, some constructions like asreturn 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.

Copy link

eugene-matvejevJul 13, 2016•
edited
Loading

Choose a reason for hiding this comment

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

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 :)

Copy link

Member

fabpotJul 15, 2016

Choose a reason for hiding this comment

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

Please, define "clean code" in this context.

Copy link

Contributor

theofidryJul 15, 2016•
edited
Loading

Choose a reason for hiding this comment

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

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