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

PERF: get_requirement() raise cache from 2048 to 8192 elements #12873

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

morotti
Copy link
Contributor

@morotti morotti commented Jul 23, 2024

Hello,

To summarize, the cache was too small :D
Should we have a NEWS item for this? It's very internal with no relevance to end users. EDIT: I added the news item since the CI was red
2% faster pip install on dry run, less in actual run.

You can debug the cache with print(f"parsing {req_string} {get_requirement.cache_info()}")

pip install --dry-run packageswith300dependencies
run with a default size
parsing python-jose[cryptography] CacheInfo(hits=51941, misses=13580, maxsize=2048, currsize=2048)

run with a larger size. 
parsing cryptography>=3.4.0; extra == "crypto" CacheInfo(hits=58758, misses=6947, maxsize=10000, currsize=6692)

before:
Screenshot 2024-07-23 114336

after:
Screenshot 2024-07-23 114343

Regards.

@morotti morotti force-pushed the perf-requirement-cache-size branch from b9eef58 to 04b2630 Compare July 23, 2024 11:26
@notatallshaw
Copy link
Member

FWIW, this, I think, was clear when it was changed to 2048: #12663 (comment)

Originally when packaging 22.0+ was going to be vendored the plan was to drop the cache altogether. But as I showed in that PR, there are real world highly esoteric backtracking scenarios where it makes a noticable difference.

I don't have an opinion on if this is a good change or not, but I do think it would be worth running a memory profiler, like memray to see it changes the peak memory usage (though due to #12834 it will be a small fraction of the total).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants