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

Performance issues after 1.3.54 #517

Closed
derrabus opened this issue Jan 16, 2024 · 19 comments
Closed

Performance issues after 1.3.54 #517

derrabus opened this issue Jan 16, 2024 · 19 comments

Comments

@derrabus
Copy link

Upgrading this library from 1.3.53 to 1.3.54 and beyond caused serious performance issues for us.

When running PHPStan with 1.3.x-dev of the plugin on my laptop, PHPStan's progress bar is stuck at 98% for 5 minutes, but the process completes eventually. With 1.3.53, PHPStan completes in 20 seconds on an empty result cache. On our CI, the PHPStan job timed out regularly which hasn't been a problem before.

With git bisect, I could identify a9d0aaf (#500, @janedbal) as the casue of this issue. If I revert this very commit on the 1.3.x branch, everything's back to normal. Our workaround for now is to pin phpstan-doctrine to the 1.3.53 release.

Unfortunately, I cannot publish the codebase I'm experiencing this issue on. I'm also a bit lost about how I could isolate a reproducer here, but I'm willing to answer any questions that might help someone investigating the problem.

@ondrejmirtes
Copy link
Member

Hi, this sounds like it gets stuck on a single file where the complexity explodes. If you run PHPStan with -vvv --debug you'll find that file. If you then simplify the file and other files this one calls (files with method declarations this file calls), you'll find the code pattern the extension gets stuck on.

@ondrejmirtes
Copy link
Member

Hey, I just reverted a9d0aaf and going to release that. I'd still appreciate if you were to point down the performance hog in 1.3.54.

I also experienced slowdown in real-world project but not as serious.

@janedbal
Copy link
Contributor

Idea: I can imagine similar behaviour, if the codebase would contain some really huge chain of methodcalls which return QueryBuilder. The differece of behaviour caused by this MR is best described in #499.

The way it works is that it deeply jumps to every method that returns QueryBuilder. And if that one contains method call that returns QueryBuilder, it jumps even there. So some huge chain of those might slow down the analysis significantly.

@janedbal
Copy link
Contributor

Instead of reverting, I think it should be possible to limit the depth of this chain @ondrejmirtes

With revert, nobody ever finds out where the problem resides and this significant improvement will never be present in this plugin imo

@ondrejmirtes
Copy link
Member

I'm usually for causing a bit of pain to force people to report problems so that the software can get better for everyone later.

But in this case I'm seeing 50 % slowdown with no change in analysis precision on a real-world 1M+ LoC codebase. So I decided to revert it.

People can still install 1.3.56 and debug where the slowdown really is. That's why I didn't close this issue.

@janedbal
Copy link
Contributor

I still think this brings significant value and (for example) adding an option to enable it (disable by default instead of revert) would be valueable for a lot of codebases.

Personally, I can copy the extension to our codebase to keep using it, but nobody else can benefit from that...

@janedbal
Copy link
Contributor

Just measured on our ~2M LoC codebase (600 doctrine entities). Compared to 1.3.56:

  • 1.3.57: slightly longer runtime, 52 new errors Could not analyse QueryBuilder (using reportDynamicQueryBuilders: true)

So I cannot debug the issue you are facing, but I can confirm it significantly helps in some codebases.

@janedbal
Copy link
Contributor

So I'd really appreciate any example test / repo where I could detect the core issue @derrabus

@janedbal
Copy link
Contributor

I tried to implement some caching in #518 (which might help a lot in codebases where you some chained createBaseQueryBuilder is called ton of times).

If somebody could confirm that it helps/does not help, it would be great. Thx

@ondrejmirtes
Copy link
Member

Please test 1.3.x-dev again, there's an improved version.

@derrabus
Copy link
Author

I've tested 1.3.x-dev (03b7d3f) on our codebase and there's no measurable difference to 1.3.53. Thank you very much for resolving this issue so quickly despite my poorly written bug report. 🚀

@ondrejmirtes
Copy link
Member

Awesome, I'm gonna release this and we'll see if someone else finds degraded performance :) Thanks everyone.

@mitelg
Copy link
Contributor

mitelg commented Jan 17, 2024

Hey there 👋

I had the same problem after updating to 1.3.54 in https://github.com/shopware5/shopware
1.3.57 was okay again, but same problem with 1.3.58

After letting it run for some time this is the error output:

-- ------------------------------------------------------------------------------------------------------------------------- 
     Error                                                                                                                    
 -- ------------------------------------------------------------------------------------------------------------------------- 
     Reached internal errors count limit of 50, exiting...                                                                    
     Internal error: Child process timed out after 600.0 seconds. Try making it longer with parallel.processTimeout setting.  
     Child process error (exit code 139): Segmentation fault (core dumped)                                                    
                                                                                                                              
     Child process error (exit code 139): Segmentation fault (core dumped)                                                    
                                                                                                                              
     Child process error (exit code 139): Segmentation fault (core dumped)                                                    
                                                                                                                              
     Child process error (exit code 139): Segmentation fault (core dumped)                                                    
                                                                                                                              
     Child process error (exit code 139): Segmentation fault (core dumped)                                                    
                                                                                                                              
 -- ------------------------------------------------------------------------------------------------------------------------- 

So I am sorry to say, it is not fixed yet for us 🙈

@ondrejmirtes
Copy link
Member

/cc @janedbal This is definitely the recursion problem I talked about.

@janedbal
Copy link
Contributor

@ondrejmirtes @mitelg Yeah, segfault is callstack overflow. Let me add some recursion prevention.

@janedbal
Copy link
Contributor

@ondrejmirtes Here it is #521

@ondrejmirtes
Copy link
Member

Gonna release the recursion fix shortly as 1.3.59. Please test it.

@mitelg
Copy link
Contributor

mitelg commented Jan 18, 2024

works! 🎉

thanks ❤️ no I have same issues to fix 😄

Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants