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

Cache Path.is_dir calls #779

Merged
merged 4 commits into from
Oct 20, 2024
Merged

Cache Path.is_dir calls #779

merged 4 commits into from
Oct 20, 2024

Conversation

ethantkoenig
Copy link
Contributor

Profiles of a poetry lock --no-update command (run on a closed-source repo I am unable to share) are showing that this Path.is_dir call is a hotspot (~0.8s out of a ~4.8s), and calls are often made with the same path multiple times (~1260 calls with ~440 unique paths):

if p.is_dir() and (os.path.sep in name or name.startswith(".")):

Caching Path.is_dir trims roughly ~0.5s off of the time spent in Path.is_dir calls.

(I'm not familiar with this code, but the fact that is_python_project is already cached makes me assume that caching Path.is_dir calls is also safe.)

ethantkoenig and others added 2 commits October 13, 2024 22:18
Profiles of a `poetry lock --no-update` command are showing that this `Path.is_dir` call is both a hotspot (~0.8s out of a ~4.8s), and is regularly called with the same path multiple times (~1260 calls with ~440 unique paths):

https://github.com/python-poetry/poetry-core/blob/1bdbd900c4fc1e2d4ed1ef15dfbcafdcb66dd892/src/poetry/core/packages/dependency.py#L364

Caching `Path.is_dir` trims roughly ~0.5s off of the time spent in `Path.is_dir` calls.
@radoering
Copy link
Member

@ethantkoenig Out of interest: Is only Path.is_dir a hotspot or is Dependency.create_from_pep_508 still an hotspot after caching Path.is_dir? It is far more risky, but I'd like to know if thinking about caching Dependency.create_from_pep_508 makes sense. We had to adapt the caching so that it returns a copy (Dependency.clone) of the dependency similar as in https://stackoverflow.com/questions/54909357/how-to-get-functools-lru-cache-to-return-new-instances so it might not make sense in the end.

@ethantkoenig
Copy link
Contributor Author

@ethantkoenig Out of interest: Is only Path.is_dir a hotspot or is Dependency.create_from_pep_508 still an hotspot after caching Path.is_dir? It is far more risky, but I'd like to know if thinking about caching Dependency.create_from_pep_508 makes sense. We had to adapt the caching so that it returns a copy (Dependency.clone) of the dependency similar as in https://stackoverflow.com/questions/54909357/how-to-get-functools-lru-cache-to-return-new-instances so it might not make sense in the end.

With this change, create_from_pep_508 is taking ~0.4s, with ~0.2s spent in cached_is_dir/is_dir

@radoering radoering merged commit 737a203 into python-poetry:main Oct 20, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants