Skip to content

[EventDispatcher] collect called listeners information only once #31991

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[EventDispatcher] collect called listeners information only once #31991

wants to merge 1 commit into from

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Jun 11, 2019

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

@xabbuh xabbuh added this to the next milestone Jun 11, 2019
@javiereguiluz
Copy link
Member

I've profiled the same app again after applying this patch:

Impressive 😱

image

Thanks a lot Christian!

@derrabus
Copy link
Member

Can't we target 4.2 with this change?

@Simperfit
Copy link
Contributor

if this has been introduced in 4.2 I think this should target 4.2 as this is more than a bugfix !

@javiereguiluz
Copy link
Member

In the past, our policy with performance improvements was:

  • Never merge them in older branches. It's tempting to do ... but at the same time, it's too easy to introduce bugs.
  • Only merging in "next" branch is great to promote the performance improvements of the new version (and encourage upgrading).

Things can change though if this is considered a bug fix.

nicolas-grekas
nicolas-grekas approved these changes Jun 11, 2019
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(I'd be fine with 4.2 :) )

lyrixx
lyrixx approved these changes Jun 11, 2019
View reviewed changes
Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

👍 for 4.2 too

@fabpot fabpot changed the base branch from 4.4 to 4.3 June 11, 2019 18:34
@fabpot
Copy link
Member

fabpot commented Jun 11, 2019

Thank you @xabbuh.

fabpot added a commit that referenced this pull request Jun 11, 2019
@fabpot
bug #31991 [EventDispatcher] collect called listeners information onl…
ac1a660 
…y once (xabbuh)

This PR was merged into the 4.3 branch.

Discussion
----------

[EventDispatcher] collect called listeners information only once

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

Commits
-------

284262a collect called listeners information only once
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Jun 11, 2019
@fabpot fabpot closed this Jun 11, 2019
@noniagriconomie noniagriconomie mentioned this pull request Jun 20, 2019
Closed
@xabbuh xabbuh mentioned this pull request Jun 25, 2019
Merged
@xabbuh xabbuh deleted the issue-31970 branch June 25, 2019 07:47
@fabpot fabpot mentioned this pull request Jun 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@lyrixx lyrixx lyrixx approved these changes

@Simperfit Simperfit Simperfit approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
EventDispatcher Feature Performance Status: Reviewed
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
9 participants
@xabbuh @javiereguiluz @derrabus @Simperfit @fabpot @nicolas-grekas @lyrixx @chalasr @carsonbot