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

nicolas-grekas · Nov 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 to Closure::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.

ostrolucky

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

nicolas-grekas · Nov 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 of getListeners().
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.

ostrolucky · Nov 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 into sorted property, you can still put it to optimized, but from inside sortListeners. That wouldn't change anything on public side.

nicolas-grekas · Nov 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)

ostrolucky · Nov 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.

nicolas-grekas · Nov 18, 2018

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

ostrolucky · Nov 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

nicolas-grekas · Nov 18, 2018

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

src/Symfony/Component/EventDispatcher/EventDispatcher.php

ostrolucky · Nov 19, 2018

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.

nicolas-grekas · Nov 20, 2018

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.

ostrolucky · Nov 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 would if ($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().

stof · Nov 20, 2018

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

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

ostrolucky · Nov 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 that wink wink

nicolas-grekas · Nov 21, 2018

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

Don't be fooled by the diff stat, I just renamed AbstractEventDispatcherTest to EventDispatcherTest and added this test case.

ostrolucky · Nov 24, 2018

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

…mCallable()

…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 proof Fabpot failure unrelated Commits ------- 0c6ef01 Optimize perf by replacing call_user_func with dynamic vars

…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 symfony/symfony#29245, but on more places and without complexity increase. Check eg. https://github.com/fab2s/call_user_func for proof Fabpot failure unrelated Commits ------- 0c6ef01713 Optimize perf by replacing call_user_func with dynamic vars

…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 symfony/symfony#29245, but on more places and without complexity increase. Check eg. https://github.com/fab2s/call_user_func for proof Fabpot failure unrelated Commits ------- 0c6ef01 Optimize perf by replacing call_user_func with dynamic vars

fabpot · Dec 10, 2018

Thank you @nicolas-grekas.

… Closure::fromCallable() (nicolas-grekas) This PR was merged into the 4.3-dev branch. Discussion ---------- [EventDispatcher][VarDumper] optimize perf by leveraging Closure::fromCallable() | 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 to `Closure::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. Commits ------- d6a594b [EventDispatcher][VarDumper] optimize perf by leveraging Closure::fromCallable()

nicolas-grekas added this to the next milestone Nov 17, 2018

carsonbot added Status: Needs Review EventDispatcher VarDumper labels Nov 17, 2018

nicolas-grekas force-pushed the callable-optim branch 3 times, most recently from 0683f6c to f157a24 Compare November 17, 2018 10:44

carsonbot removed the Status: Needs Review label Nov 18, 2018

ostrolucky suggested changes Nov 18, 2018

View reviewed changes

carsonbot added the Status: Needs Work label Nov 18, 2018

nicolas-grekas force-pushed the callable-optim branch from f157a24 to 2b9d9c8 Compare November 18, 2018 09:48

nicolas-grekas added Status: Needs Review and removed Status: Needs Work labels Nov 18, 2018

nicolas-grekas force-pushed the callable-optim branch from 2b9d9c8 to 95eb251 Compare November 18, 2018 10:11

nicolas-grekas force-pushed the callable-optim branch 2 times, most recently from 5a9ac14 to 5bc59f1 Compare November 18, 2018 10:29

nicolas-grekas force-pushed the callable-optim branch from 5bc59f1 to 719e602 Compare November 18, 2018 10:56

ostrolucky reviewed Nov 18, 2018

View reviewed changes

src/Symfony/Component/EventDispatcher/EventDispatcher.php Show resolved Hide resolved

nicolas-grekas force-pushed the callable-optim branch 3 times, most recently from 446270e to 68f3b51 Compare November 21, 2018 14:31

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 the callable-optim branch from 68f3b51 to d6a594b Compare December 1, 2018 09:36

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 10, 2018

fabpot approved these changes Dec 10, 2018

View reviewed changes

fabpot merged commit d6a594b into symfony:master Dec 10, 2018

nicolas-grekas deleted the callable-optim branch January 25, 2019 14:27

nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

derrabus mentioned this pull request May 16, 2019

[EventDispatcher] Removed "callable" type hint from WrappedListener constructor #31493

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

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

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

Uh oh!

Conversation

nicolas-grekas commented Nov 17, 2018

Uh oh!

ostrolucky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicolas-grekas commented Nov 18, 2018

Uh oh!

ostrolucky commented Nov 18, 2018

Uh oh!

nicolas-grekas commented Nov 18, 2018

Uh oh!

ostrolucky commented Nov 18, 2018

Uh oh!

nicolas-grekas commented Nov 18, 2018

Uh oh!

ostrolucky commented Nov 18, 2018

Uh oh!

nicolas-grekas commented Nov 18, 2018

Uh oh!

Uh oh!

ostrolucky commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ostrolucky commented Nov 20, 2018

Uh oh!

stof commented Nov 20, 2018

Uh oh!

ostrolucky commented Nov 20, 2018

Uh oh!

nicolas-grekas commented Nov 21, 2018

Uh oh!

ostrolucky commented Nov 24, 2018

Uh oh!

fabpot commented Dec 10, 2018

Uh oh!

Uh oh!

ostrolucky commented Nov 19, 2018 •

edited

Loading

nicolas-grekas commented Nov 20, 2018 •

edited

Loading