-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: BracesPositionFixer - fix performance issue for massive files with CT::T_CURLY_CLOSE #8830
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
…e handle all cases, not only those we remembered about
Pull request was converted to draft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about instead of this file, which is 1.7MB, we add a test case like below?
yield 'string with a lot of variables in braces' => [
'<?php
if (true) {
$foo = "' . str_repeat(' text {$variable} text ', 50_000) . '";
}',
];
Especially that the name of the test case is a spoiler for what is coming next: the current fix works for 👆🏼 , but not for 👇🏼 .
yield 'string with a lot of variables' => [
'<?php
if (true) {
$foo = "' . str_repeat(' text $variable text ', 50_000) . '";
}',
];
Which would need another over 1MB if we go in the direction of explicit files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh, i'm ok with both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think to move back to integration test.
CI is failing when code-coverage or mutation, and integration test is outside of them by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the same problem exists for concatenation of many variables..
yield 'string with concatenation of a lot of variables' => [
'<?php
if (true) {
$foo = "' . str_repeat(' text ".$variable." text ', 50_000) . '";
}',
];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think to move back to integration test. CI is failing when code-coverage or mutation, and integration test is outside of them by default
done
$openBraceIndex = $tokens->getNextTokenOfKind($index, ['{', ';', [CT::T_CURLY_CLOSE], [CT::T_PROPERTY_HOOK_BRACE_OPEN], [T_CLOSE_TAG]]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$openBraceIndex = $tokens->getNextTokenOfKind($index, ['{', ';', [CT::T_CURLY_CLOSE], [CT::T_PROPERTY_HOOK_BRACE_OPEN], [T_CLOSE_TAG]]); | |
$nextToken = $tokens->getNextMeaningfulToken($index); | |
if (!$tokens[$nextToken]->equalsAny(['=', [CT::T_PROPERTY_HOOK_BRACE_OPEN]])) { | |
continue; | |
} | |
$openBraceIndex = $tokens->getNextTokenOfKind($x, ['{', ';', [CT::T_PROPERTY_HOOK_BRACE_OPEN], [T_CLOSE_TAG]]); |
I was thinking about something like this:
- first - quick "deal breaker" for having a property hook after the variable
- second - proper check if the property hook is there
The $x
could be either $index
(for simplicity) or $nextToken - 1
(for microoptimization).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if I grasp your idea here, want to sent a PR with it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the variable is not followed by =
or T_PROPERTY_HOOK_BRACE_OPEN
, it cannot be a property with hooks.
In the given test cases it would be a fast fail for all variables inside the string.
Only for the first $foo = ...
variable it would still traverse through all tokens in the long statement to the statement end (;
).
Co-authored-by: Kuba Werłos <werlos@gmail.com>
I'm thinking to merge and release before my hols, so production setup is bit better. |
closes #8828
interesting part is simply that utest work, just super slow if too many tokens with
{$foo}
so utest not failing, but itest can run out of time