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 only load files once #8011

Merged
merged 2 commits into from May 26, 2022

Conversation

kkmuffme
Copy link
Contributor

Fixes #8008

Speeds up psalm between 5-15%.

Also fixes an issue on Unix systems where A.php and a.php were treated equal, even though they are different files, since unix file names are case sensitive.

@kkmuffme kkmuffme force-pushed the performance-only-load-files-once branch 3 times, most recently from 248d1f0 to 17a8426 Compare May 25, 2022 11:20
* use static to keep opened files with content
* move position of file cache population to the place where we read files to ensure cache always gets populated and not on open only (since it's called directly in some places)
@kkmuffme kkmuffme force-pushed the performance-only-load-files-once branch from 17a8426 to 06178d0 Compare May 25, 2022 11:54
@kkmuffme kkmuffme force-pushed the performance-only-load-files-once branch from 741c32e to 278e877 Compare May 25, 2022 12:12
@AndrolGenhald
Copy link
Collaborator

Can you double check memory usage? I'd expect the parsed structures to be much bigger than file contents, but this will definitely increase memory usage and it wouldn't be bad to double check. I assume this code is bypassed for the language server, but that would be good to verify as well.

@AndrolGenhald AndrolGenhald added the release:internal The PR will be included in 'Internal changes' section of the release notes label May 25, 2022
@kkmuffme
Copy link
Contributor Author

The memory usage depends on what you are checking, as the memory usage is directly proportional to the number and size of files analyzed.

If real world tests show that memory consumption is too high, we could offer an option to
a) limit the number of files that stay "cached" or
b) only "cache" specific types (e.g. stubs)
c) exclude certain files from "cache"

I tested it on the WP core repo and the memory usage was only slightly higher than before.

@AndrolGenhald
Copy link
Collaborator

Yeah, that's what I expected, just wanted to confirm with an actual test.

I do want to double check how the language server works though. If the language server relies on this class then the caching could completely break it, unless it's only used to get initial file state in which case the cache is just wasting memory.

@weirdan
Copy link
Collaborator

weirdan commented May 25, 2022

Language server should also support the usecase when the file is not saved yet, so it shouldn't rely really on files on disk anyway.

@kkmuffme
Copy link
Contributor Author

Could someone merge this?

@orklah orklah merged commit 06d8e3e into vimeo:4.x May 26, 2022
@orklah
Copy link
Collaborator

orklah commented May 26, 2022

Thanks!

@kkmuffme kkmuffme deleted the performance-only-load-files-once branch May 28, 2022 22:37
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.

Performance: only load files once
4 participants