Movatterモバイル変換


[0]ホーム

URL:


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

Sign up
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:4.1fromostrolucky:feature-callable
Dec 10, 2018

Conversation

@ostrolucky
Copy link
Contributor

@ostroluckyostrolucky commentedNov 25, 2018
edited
Loading

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
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

Koc reacted with thumbs up emoji
@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 commentedNov 25, 2018
edited
Loading

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. seehttps://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 nofoo::bar style can possibly be used) and merge them on 3.4.

@ostrolucky
Copy link
ContributorAuthor

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 assumefoo::bar is not used

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedNov 25, 2018
edited
Loading

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

@ostrolucky
Copy link
ContributorAuthor

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
ContributorAuthor

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

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.

@ostrolucky
Copy link
ContributorAuthor

ostrolucky commentedDec 6, 2018
edited
Loading

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)

@chalasrchalasr added this to the4.1 milestoneDec 9, 2018
@ostroluckyostrolucky changed the base branch frommaster to4.1December 9, 2018 21:05
@ostrolucky
Copy link
ContributorAuthor

rebased, only fabpot failures not cause by my changes

@fabpot
Copy link
Member

Thank you@ostrolucky.

@fabpotfabpot merged commit0c6ef01 intosymfony:4.1Dec 10, 2018
fabpot added a commit that referenced this pull requestDec 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 proofFabpot failure unrelatedCommits-------0c6ef01 Optimize perf by replacing call_user_func with dynamic vars
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

5 participants

@ostrolucky@nicolas-grekas@fabpot@chalasr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp