-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
AFAIK, these functions have no overheard when they're called with a |
Verified here also, measurable perf gain confirmed (<10%). |
What we could do is wonder case by case when such a change is safe doing for 5.5 (ie no |
Whatever the tool, not having to resolve anything is always going to be faster :) |
You can even configure it to do this automatically every time without asking ;) |
Closing as explained, thanks for submitting. |
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. |
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 |
Then let's merge this in 4.1? 55 files modified, that's a lot of potential future conflicts I fear. |
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? |
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
to
4ba5806
Compare
4ba5806
to
04c7558
Compare
04c7558
to
0c6ef01
Compare
rebased, only fabpot failures not cause by my changes |
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 proof Fabpot failure unrelated Commits ------- 0c6ef01 Optimize perf by replacing call_user_func with dynamic vars
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