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

store origin location by ID to speed up psalm by up to 75% #8054

Merged
merged 1 commit into from
Jun 4, 2022

Conversation

kkmuffme
Copy link
Contributor

@kkmuffme kkmuffme commented Jun 4, 2022

In some instances with particularly nested/bad code, psalm can spend minutes on a single file (and it's includes) with getOriginLocations being called over and over again on the same thing.
This runtime caches the locations in a private variable, to speed up psalm up to 75% (worst file I tested)

Similar to #8011

@kkmuffme
Copy link
Contributor Author

kkmuffme commented Jun 4, 2022

This is ready to be merged :)

@orklah
Copy link
Collaborator

orklah commented Jun 4, 2022

Could you add the psalm report with a before/after on a big enough project so we see the difference in memory consumption please?

@kkmuffme
Copy link
Contributor Author

kkmuffme commented Jun 4, 2022

With 2455 errors found in the codebase:

From:

Checks took 242.62 seconds and used 1,059.082MB of memory

To:

Checks took 66.33 seconds and used 1,059.087MB of memory

@orklah
Copy link
Collaborator

orklah commented Jun 4, 2022

can you make sure to run without cache?

I find this suspicious, VariableUseGraph is an object that is kept all along analysis. Caching things in here shoud be visible in the memory usage

@kkmuffme
Copy link
Contributor Author

kkmuffme commented Jun 4, 2022

Did some more testing, and it seems this issue arises from the use of psalm plugins where CodeLocation is used.
(these are not comparable to the previous comment, as it was run on a faster instance, but I did everything from scratch.)

I tried it with the base config (no stubs, no plugins,...) for the codebase I tested above + another bigger code base now.
Also I tested with --no-cache and --threads=1

BASE CONFIG
From:

Checks took 27.32 seconds and used 402.501MB of memory
Checks took 67.72 seconds and used 1,838.005MB of memory

To:

Checks took 27.27 seconds and used 402.501MB of memory
Checks took 67.85 seconds and used 1,838.005MB of memory

I tried it on my biggest codebase available (11k files), but there psalm has another bug #7863 so it failed.

It seems like there is no impact on performance and memory consumption with the default psalm config at all.
How this is possible? getOriginLocations never gets called with psalm base config.

CUSTOM CONFIG WITH PLUGINS
Again without cache, single threaded.

From:

Checks took 207.57 seconds and used 857.046MB of memory

To:

Checks took 46.30 seconds and used 859.465MB of memory

Memory consumption increased by 0.3%, speed increased by ~78%

@orklah
Copy link
Collaborator

orklah commented Jun 4, 2022

IIRC, VariableUseGraph is used for two things: taint detection and unused variable detection. If you base config don't use unused var detection nor taint detection, it may not be called at all.

Then, maybe some plugins or some config in your config file enabled one of the two feature and made use of VariableUseGraph

@kkmuffme
Copy link
Contributor Author

kkmuffme commented Jun 4, 2022

Yeah, thanks. I don't manage the psalm configs/plugins for our team, just doing devops performance work.

While there is an increase in memory consumption that is linear to the number of files analyzed (and to some extent to what is in those files), the increase in performance is completely worth it in my opinion.
I tried a few additional codebases now, and in all cases the speed increase surpassed the increased memory consumption manifold.

@orklah orklah added the release:internal The PR will be included in 'Internal changes' section of the release notes label Jun 4, 2022
@orklah orklah merged commit f47b418 into vimeo:4.x Jun 4, 2022
@orklah
Copy link
Collaborator

orklah commented Jun 4, 2022

Thanks!

@kkmuffme kkmuffme deleted the runtime-cache-origin-location branch June 4, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:internal The PR will be included in 'Internal changes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants