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

[EventDispatcher][VarDumper] optimize perf by leveraging Closure::fromCallable()#29245

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

Merged

fabpot merged 1 commit intosymfony:masterfromnicolas-grekas:callable-optim

Dec 10, 2018

Merged

[EventDispatcher][VarDumper] optimize perf by leveraging Closure::fromCallable()#29245

fabpot merged 1 commit intosymfony:masterfromnicolas-grekas:callable-optim

Dec 10, 2018

Conversation

Copy link

Member

nicolas-grekas commentedNov 17, 2018

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

Callables are notably slower than closures. Let's turn then to closures thanks toClosure::fromCallable().

This doesn't affect performance for run-once listeners.
And improves performance for events that are dispatched several times.

Same for VarDumper's casters.

nicolas-grekas added this to thenext milestone

Nov 17, 2018

carsonbot added Status: Needs Review EventDispatcher VarDumper labels

Nov 17, 2018

nicolas-grekas force-pushed thecallable-optim branch 3 times, most recently from0683f6c tof157a24Compare

November 17, 2018 10:44

carsonbot removed the Status: Needs Review label

Nov 18, 2018

ostrolucky suggested changes

Nov 18, 2018

View reviewed changes

Copy link

Contributor

ostrolucky left a comment

Choose a reason for hiding this comment

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

Why is this not incorporated inside sortListeners function? Instead it repeats the logic but ignores sort marking and is used only during dispatching

This doesn't affect performance for run-once listeners.

How come? It makes extra operations (new closure, Closure::fromCallable call)

src/Symfony/Component/EventDispatcher/EventDispatcher.php OutdatedShow resolvedHide resolved

src/Symfony/Component/EventDispatcher/EventDispatcher.phpShow resolvedHide resolved

src/Symfony/Component/EventDispatcher/EventDispatcher.php OutdatedShow resolvedHide resolved

carsonbot added the Status: Needs Work label

Nov 18, 2018

nicolas-grekas force-pushed thecallable-optim branch fromf157a24 to2b9d9c8Compare

November 18, 2018 09:48

Copy link

MemberAuthor

nicolas-grekas commentedNov 18, 2018

@ostrolucky thanks for the review!

Why is this not incorporated inside sortListeners function? Instead it repeats the logic but ignores sort marking and is used only during dispatching

Because sortListeners has public side effect: it drive the return value ofgetListeners().
Changing would mean breaking BC.

How come? It makes extra operations (new closure, Closure::fromCallable call)

and removes some. I did benchmark this, there is no measurable difference.

nicolas-grekas added Status: Needs Review and removed Status: Needs Work labels

Nov 18, 2018

Copy link

Contributor

ostrolucky commentedNov 18, 2018

it drive the return value of getListeners()

getListeners() should continue returning the exact listeners that where added

I didn't mean to to imply you should save the optimization result intosorted property, you can still put it tooptimized, but from inside sortListeners. That wouldn't change anything on public side.

nicolas-grekas force-pushed thecallable-optim branch from2b9d9c8 to95eb251Compare

November 18, 2018 10:11

Copy link

MemberAuthor

nicolas-grekas commentedNov 18, 2018

you can still put it to optimized, but from inside sortListeners

that could defeat the extra laziness added here (not instantiating lazy listeners when propagation is stopped before them)

nicolas-grekas force-pushed thecallable-optim branch 2 times, most recently from5a9ac14 to5bc59f1Compare

November 18, 2018 10:29

Copy link

Contributor

ostrolucky commentedNov 18, 2018

that could defeat the extra laziness added here (not instantiating lazy listeners when propagation is stopped before them)

I don't see how would it defeat it 🤔 Only way that closure is replaced with delegated one is when it's executed, which will happen only during dispatching.

Copy link

MemberAuthor

nicolas-grekas commentedNov 18, 2018

Because of the$listener[0] = $listener[0](); line, which is the one that is made lazy here.

nicolas-grekas force-pushed thecallable-optim branch from5bc59f1 to719e602Compare

November 18, 2018 10:56

Copy link

Contributor

ostrolucky commentedNov 18, 2018

I get that, but that line is executed only when outer closure is executed. I don't see why would moving construction of this closure to different place change when is it executed

Copy link

MemberAuthor

nicolas-grekas commentedNov 18, 2018

Because we have to call that line to populate the sortedListeners property.

ostrolucky reviewed

Nov 18, 2018

View reviewed changes

src/Symfony/Component/EventDispatcher/EventDispatcher.phpShow resolvedHide resolved

Copy link

Contributor

ostrolucky commentedNov 19, 2018•
edited
Loading

This closure trick increases speed by 0.6μs aggregated (8%). 0.15μs from that is just thanks to inlining getListeners call. So closure trick alone is 0.45μs. Measured with phpbench, 100 listeners attached (and all of them executed). Correlation on number of listeners is very linear.

I don't think this is worth increased complexity and number of untested paths. Also, is there a PHP bug report? I think this is something better to be improved on language level.

Copy link

MemberAuthor

nicolas-grekas commentedNov 20, 2018•
edited
Loading

We made current Symfony versions really fast because we made such changes. Small improvements for your bench can be a significant one for a specific use case. And it's already been proven that many small improvements aggregate to significant general ones.

The real issue is having EventDispatcher mutable. We should inspect existing use cases where this is needed (if any) and consider deprecating if possible IMHO. But that shouldn't block this one.

Copy link

Contributor

ostrolucky commentedNov 20, 2018

Well if we decide it's worth it, can you at least properly comment this code, cover it by tests and notify upstream about workarounds we need to do to increase performance so there is at least some chance we can eventually remove them? This was proven successful with eg inlining constants in DI Container. Performance tweaks like this are killing readability and hence ability to debug and contribute a lot. As you can see from my questions there are lot of non-obvious things for other people.

I still don't know in what case wouldif ($listener[0] instanceof \Closure) { condition be needed, because it's not covered by tests. And there is no longer even coverage for calling getListeners() call from dispatch().

Copy link

Member

stof commentedNov 20, 2018

I still don't know in what case wouldif ($listener[0] instanceof \Closure) { condition be needed, because it's not covered by tests.

that part is the lazy-loading of the listener object.

Copy link

Contributor

ostrolucky commentedNov 20, 2018

I see that, I just don't see how can be listener modified outside of this proxy closure. But testcase would help me with thatwink wink

nicolas-grekas force-pushed thecallable-optim branch 3 times, most recently from446270e to68f3b51Compare

November 21, 2018 14:31

Copy link

MemberAuthor

nicolas-grekas commentedNov 21, 2018

The discussed code path is now tested@ostrolucky, testMutatingWhilePropagationIsStopped.

Don't be fooled by the diff stat, I just renamedAbstractEventDispatcherTest toEventDispatcherTest and added this test case.

Copy link

Contributor

ostrolucky commentedNov 24, 2018

Thanks. I can see now that same closure is evaluated by sortListeners and optimizeListeners might not re-triggered after that happens.

ostrolucky mentioned this pull request

Nov 25, 2018

Optimize perf by replacing call_user_func with dynamic variables#29309

Merged

[EventDispatcher][VarDumper] optimize perf by leveraging Closure::fro…

d6a594b

…mCallable()

nicolas-grekas force-pushed thecallable-optim branch from68f3b51 tod6a594bCompare

December 1, 2018 09:36

fabpot added a commit that referenced this pull request

Dec 10, 2018

minor#29309Optimize perf by replacing call_user_func with dynamic v…

4c08b07

…ariables (ostrolucky)This PR was merged into the 4.1 branch.Discussion----------Optimize perf by replacing call_user_func with dynamic variables| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |This provides similar boost as in#29245, but on more places and without complexity increase. Check eg.https://github.com/fab2s/call_user_func for proofFabpot failure unrelatedCommits-------0c6ef01 Optimize perf by replacing call_user_func with dynamic vars

symfony-splitter pushed a commit to symfony/cache that referenced this pull request

Dec 10, 2018