Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
merged 1 commit into from
Dec 10, 2018

Conversation

ostrolucky
Copy link
Contributor

@ostrolucky ostrolucky commented Nov 25, 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

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

@nicolas-grekas
Copy link
Member

AFAIK, these functions have no overheard when they're called with a \. That's because they're resolved at compile time. Would you mind verifying please?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 25, 2018

Verified here also, measurable perf gain confirmed (<10%).
Actually, we wondered about this change before and concluded we cannot do it because of PHP5.5 support: call_user_func($a) accepts more callables than $a() in PHP5.5. see https://3v4l.org/NGGth
Then, merging this into master is technically possible, but would open for years of merge conflicts we'll have to deal with weekly. I'd better not.

@nicolas-grekas
Copy link
Member

What we could do is wonder case by case when such a change is safe doing for 5.5 (ie no foo::bar style can possibly be used) and merge them on 3.4.

@ostrolucky
Copy link
Contributor Author

How do you solve these conflicts? Intellij knows how to resolve such simple conflicts automatically.
peek 2018-11-25 10-39

I don't think there are much cases when we can safely assume foo::bar is not used

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 25, 2018

Whatever the tool, not having to resolve anything is always going to be faster :)

@ostrolucky
Copy link
Contributor Author

You can even configure it to do this automatically every time without asking ;)

@nicolas-grekas
Copy link
Member

Closing as explained, thanks for submitting.

@ostrolucky
Copy link
Contributor Author

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
Copy link
Member

fabpot commented Dec 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
Copy link
Member

Then let's merge this in 4.1? 55 files modified, that's a lot of potential future conflicts I fear.

@nicolas-grekas nicolas-grekas reopened this Dec 1, 2018
@ostrolucky
Copy link
Contributor Author

ostrolucky commented Dec 6, 2018

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
Copy link
Member

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)

@chalasr chalasr added this to the 4.1 milestone Dec 9, 2018
@ostrolucky ostrolucky changed the base branch from master to 4.1 December 9, 2018 21:05
@ostrolucky
Copy link
Contributor Author

rebased, only fabpot failures not cause by my changes

@fabpot
Copy link
Member

fabpot commented Dec 10, 2018

Thank you @ostrolucky.

@fabpot fabpot merged commit 0c6ef01 into symfony:4.1 Dec 10, 2018
fabpot added a commit that referenced this pull request Dec 10, 2018
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.