Skip to content
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

fix: Faster StatementIndentationFixer #7957

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Apr 18, 2024

when re-ordering the conditions we highly reduce the number of function calls happening in a regular code base.

As PHP (tested on 8.2.12) is pretty fast, the change itself does not yield much improvements regarding runtime.

As soon as php-extensions like xdebug, blackfire or other dev-tools which incur a perf penalty on function calls are in the game we see improvements though.

running php-cs-fixer on one of our closed source projects before this PR and a php.ini like

zend_extension=xdebug
xdebug.mode=debug
xdebug.start_with_request=yes

makes a big difference though (so xdebug is just loaded).

3 runs before this PR: 1m5s, 1m11s, 1m8s
3 runs after this PR: 44s, 41s, 45s

I think people having xdebug loaded while daily work while running a dev tool like php-cs-fixer is not unusal.


additionally by removing function calls we improve php-cs-fixer beeing profiled, because xdebug and blackfire currently are not able to yield useful results, as there are so many function calls happening, that the function call overhead by the profiler is more visible then the actual php-cs-fixer workload.

after this PR, as we have less function calls, profiles get a bit more precise. tbh we should do a few more optimizations to bring the tooling into a state where it can be helpful for us.

@staabm staabm changed the title Faster StatementIndentationFixer Fix: Faster StatementIndentationFixer Apr 18, 2024
$endIndex = $tokens->getNextTokenOfKind($index, ['{']);
} elseif ($token->isGivenKind(CT::T_USE_TRAIT)) {
$endIndex = $tokens->getNextTokenOfKind($index, [';']);
if ($token->equals('{')) {
Copy link
Contributor Author

@staabm staabm Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only change is re-ordering of already existing if-elseif cases so cases which are more likely to happen are checked first

@staabm staabm changed the title Fix: Faster StatementIndentationFixer fix: Faster StatementIndentationFixer Apr 18, 2024
@coveralls
Copy link

coveralls commented Apr 18, 2024

Coverage Status

coverage: 96.053%. remained the same
when pulling a64ee64 on staabm:fast-indent
into 5519faf on PHP-CS-Fixer:master.

@staabm
Copy link
Contributor Author

staabm commented Apr 18, 2024

grml.. blackfire reported a 30% faster execution, but doing some final tests without a profiler makes me sad as the profiled improvements don't show up without a profiler.

I think the problem is that cs-fixer is doing hundreds of millions function calls. and because of this huge amount of function calls the profiler overhead is bigger and the tool cannot provide meaningful results.

@staabm staabm closed this Apr 18, 2024
@staabm staabm deleted the fast-indent branch April 18, 2024 15:34
@staabm
Copy link
Contributor Author

staabm commented Apr 18, 2024

@Wirone I wonder whether a PR which reduces function calls would be acceptable - even if it does not improve performance and create a bit of class-internal code duplication - but would lead to more meaningful blackfire profiles..?

I see a lot of unnecessary function calls (e.g. proxying calls between tiny methods in Token.php)

@staabm
Copy link
Contributor Author

staabm commented Apr 18, 2024

see how frequent these top-10 functions are called

grafik

at such an high rate the blackfire profiler cannot do a good job anymore.

@Wirone
Copy link
Member

Wirone commented Apr 18, 2024

@staabm hard to tell, if it's something within a single fixer then maybe, but if it's app-wide, then it's not a question for me, as we have agreement that architectural decisions must go through @keradus 😅.

@staabm
Copy link
Contributor Author

staabm commented Apr 18, 2024

I would look into the Token.php class only, which seem to be a high roller as per screen above

@Wirone
Copy link
Member

Wirone commented Apr 18, 2024

Tokens and Token are part of the public API (used by custom fixers), so changes there have to be backward compatible. You can try, but most likely it will wait for Keradus' decision 🙂.

@staabm staabm restored the fast-indent branch April 19, 2024 06:40
@staabm staabm reopened this Apr 19, 2024
@staabm staabm marked this pull request as ready for review April 19, 2024 06:48
@staabm
Copy link
Contributor Author

staabm commented Apr 19, 2024

just re-phrased the use-case and the PR description. I think the change itself is still valuable

@mvorisek
Copy link
Contributor

I am just curious, how does Blackfire compare with xdebug? Any experience with some sampling profilers like https://github.com/nikic/sample_prof or https://github.com/reliforp/reli-prof?

@staabm
Copy link
Contributor Author

staabm commented Apr 19, 2024

I have no experience with other profilers then xdebug and blackfire

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants