Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Optimize perf by replacing call_user_func with dynamic variables#29309
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedNov 25, 2018
AFAIK, these functions have no overheard when they're called with a |
nicolas-grekas commentedNov 25, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Verified here also, measurable perf gain confirmed (<10%). |
nicolas-grekas commentedNov 25, 2018
What we could do is wonder case by case when such a change is safe doing for 5.5 (ie no |
ostrolucky commentedNov 25, 2018
nicolas-grekas commentedNov 25, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Whatever the tool, not having to resolve anything is always going to be faster :) |
ostrolucky commentedNov 25, 2018
You can even configure it to do this automatically every time without asking ;) |
nicolas-grekas commentedNov 30, 2018
Closing as explained, thanks for submitting. |
ostrolucky commentedDec 1, 2018
What makes you think this will cause conflicts on weekly basis? Those lines are rarely changed. I really don't see how are changes like#29245 preferable over this one. Performance gain is very similar but lot of places benefit from it instead of one, it has 0 complexity penalty nor change of behavior and very easy to solve conflicts. Lastly, if you are so worried about merge conflicts that are solvable automatically, you should fix your merge tool. |
fabpot commentedDec 1, 2018
I tend to agree with@ostrolucky here. I'm not sure it will be such a pain, and if the gains are substantial, it might be worth reconsidering |
nicolas-grekas commentedDec 1, 2018
Then let's merge this in 4.1? 55 files modified, that's a lot of potential future conflicts I fear. |
ostrolucky commentedDec 6, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Should I rebase? I didn't base it on older branch because I wanted to convert as much calls as possible, there are some new places in 4.2 that are missing in 4.1. Will you apply it to rest of the places during merge? |
nicolas-grekas commentedDec 9, 2018
Yes please rebase. We'll handle the 4.2-only sites either while merging or in another PR (eg you can keep this one for reference) |
55c48da to4ba5806Compare4ba5806 to04c7558Compare04c7558 to0c6ef01Compareostrolucky commentedDec 9, 2018
rebased, only fabpot failures not cause by my changes |
fabpot commentedDec 10, 2018
Thank you@ostrolucky. |
…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

Uh oh!
There was an error while loading.Please reload this page.
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