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

Larave11 makes parsing large Enum files considerably slower. #10979

Closed
Sado4 opened this issue May 9, 2024 · 15 comments · Fixed by phpstan/phpstan-src#3062
Closed

Larave11 makes parsing large Enum files considerably slower. #10979

Sado4 opened this issue May 9, 2024 · 15 comments · Fixed by phpstan/phpstan-src#3062

Comments

@Sado4
Copy link

Sado4 commented May 9, 2024

Bug report

  • Laravel Version: 11.7
  • phpstan1.10.67

Description

This was not a problem in Laravel 10, but when updating to Larave 11, stan becomes extremely slow. Specifically, what used to take around 30 seconds takes 9 minutes, etc.

Laravel code where the issue was found

Thus, it stops at 94%.

Result cache not used because the metadata do not match: projectConfig, analysedPaths
 703/743 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░░]  94% 19 secs/20 secs

I was testing with different versions and it seems that phpstan 1.10.58 does not slow down. At the time of updating from Laravel 10 to Laravel 11, phpstan is upgraded to 1.10.67, which causes the problem.

Specifically, an enum file that has over 100 cases and several methods that reference them. If necessary, I will send you the file information so you can investigate.

LibraryMasterEnum.php.zip

Code snippet that reproduces the problem

No response

Expected output

It is hoped that large Enum files will not be delayed in the latest version.

Did PHPStan help you today? Did it make you happy in any way?

No response

@ondrejmirtes
Copy link
Member

Hi, to investigate this I need something reproducible. So please create a small repository with a few files and Laravel framework where I can see how and why PHPStan's analysis is slow.

@Sado4
Copy link
Author

Sado4 commented May 9, 2024

Thanks for the reply!

Sorry I don't have time myself. The file I just sent you is all I have. Please run stan on that file and try to run each version.

Best regards.

@ondrejmirtes
Copy link
Member

Hey @staabm your PR phpstan/phpstan-src#2985 made this case a lot worse (I just bisected it), going from 3s to 23s on my machine. Can you please look into it? Thanks!

The source file reports 80 errors but it doesn't matter, the slowdown is perceivable anyway.

@staabm
Copy link
Contributor

staabm commented May 9, 2024

Will have a look tomorrow

@staabm
Copy link
Contributor

staabm commented May 10, 2024

just leaving some details here while working on it:

profile of the given example
https://blackfire.io/profiles/20da814a-2056-40c7-a892-8aec97d58971/graph

grafik

unsuprisingly cause is a big union. diving deeper now.

@ondrejmirtes
Copy link
Member

I'm not a fan of the proposed phpstan/phpstan-src#3059 and phpstan/phpstan-src#3060. They're not addressing the root cause. Once we address the root cause and the methods are called far less often, then these proposed changes are going to have minimal effect, making them not worth it.

I'd like to understand why some pieces of code like this:

if ($types[$i] instanceof EnumCaseObjectType) {
    $enumCaseTypes[$types[$i]->describe(VerbosityLevel::cache())] = $types[$i];

    unset($types[$i]);
    continue;
}

And some other pieces of code like this instead:

$enumCases = $types[$i]->getEnumCases();
if (count($enumCases) === 1) {
    $enumCaseTypes[$types[$i]->describe(VerbosityLevel::cache())] = $types[$i];

    unset($types[$i]);
    continue;
}

What's the difference with the actual objects being passed there?

@staabm
Copy link
Contributor

staabm commented May 10, 2024

yeah its something which stood out on the way to analyze the root cause. I had the impression the changes itself improve the code no matter the underlying perf problem, therefore I sent the PRs. feel free to close them.

I will submit a root-cause ortiented fix in a few minutes

@ondrejmirtes
Copy link
Member

They might be valuable but we need to measure it after the root cause fix is applied, to look at it truthfully.

@staabm
Copy link
Contributor

staabm commented May 10, 2024

What's the difference with the actual objects being passed there?

$enumCases = $types[$i]->getEnumCases();
if (count($enumCases) === 1) {
    $enumCaseTypes[$types[$i]->describe(VerbosityLevel::cache())] = $types[$i];

    unset($types[$i]);
    continue;
}

effectively filters for EnumCaseObjectType or subtracted unions which effectively only contain a single element after subtraction.

while

if ($types[$i] instanceof EnumCaseObjectType) {
    $enumCaseTypes[$types[$i]->describe(VerbosityLevel::cache())] = $types[$i];

    unset($types[$i]);
    continue;
}

does not take subtraction into account at type-combination time

@ondrejmirtes
Copy link
Member

So what makes the first code slow? $types[$i]->getEnumCases()

Could this be solved with some fast method like Type::getEnumCasesCount()?

@staabm
Copy link
Contributor

staabm commented May 10, 2024

I just realized there is another difference.

reverting phpstan/phpstan-src#2985 also makes the repro for #10979 report more errors (80 vs. 84)

@ondrejmirtes
Copy link
Member

Yeah that might be okay

@staabm
Copy link
Contributor

staabm commented May 10, 2024

So what makes the first code slow? $types[$i]->getEnumCases()

it gets slow, because of massive repetative calls into reflection, see

Could this be solved with some fast method like Type::getEnumCasesCount()?

to get this count we could get rid of some costs, but not the main cost - the repetative calls to getCases() which is the most expensive sub-call:

grafik

see the latest profile https://blackfire.io/profiles/df1197c2-f3d3-4176-894c-f3a392220fe7/graph (generated for the case in phpstan/phpstan-src#2985 after reverting the fix)

I will give it a try

@ondrejmirtes
Copy link
Member

I did this: phpstan/phpstan-src@39ce042

Can you now measure: #10772 and #10979 with the current TypeCombinator code?
And can you measure #10772 and #10979 with TypeCombinator reverted to using getEnumCases?

@staabm
Copy link
Contributor

staabm commented May 10, 2024

Our progress overlapped :-).

I completed phpstan/phpstan-src#3062 (comment) a few minutes ago which is fast for both cases.

It does not require your latest class-reflection caching commit, so you may revert it if you like

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