Skip to content

Comments

Cache Path.is_dir calls#779

Merged
radoering merged 4 commits intopython-poetry:mainfrom
ethantkoenig:main
Oct 20, 2024
Merged

Cache Path.is_dir calls#779
radoering merged 4 commits intopython-poetry:mainfrom
ethantkoenig:main

Conversation

@ethantkoenig
Copy link
Contributor

@ethantkoenig ethantkoenig commented Oct 14, 2024

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):

poetry-core/src/poetry/core/packages/dependency.py

Line 364 in 1bdbd90

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
@ethantkoenig
Cache Path.is_dir calls
381f0bf 
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.
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
16643fc 
for more information, see https://pre-commit.ci
@radoering
Copy link
Member

radoering commented Oct 15, 2024

@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 commented Oct 17, 2024

@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
@radoering radoering mentioned this pull request Dec 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@radoering radoering radoering approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ethantkoenig @radoering