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

gh-118761: improve import time for pickle #128732

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

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Jan 11, 2025

We can remove the re import which takes quite a long time. Benchmarks were performed on a RELEASE build (no PGO, no LTO). It's a bit hard to have stable numbers with -X importtime, so I'm only using the hyperfine benchmarks.

PR

$ hyperfine --warmup 8 "./python -c 'import pickle'"
Benchmark 1: ./python -c 'import pickle'
  Time (mean ± σ):       7.8 ms ±   0.3 ms    [User: 6.6 ms, System: 1.3 ms]
  Range (min … max):     7.4 ms …   9.8 ms    337 runs

Main

$ hyperfine --warmup 8 "./python -c 'import pickle'"
Benchmark 1: ./python -c 'import pickle'
  Time (mean ± σ):       9.7 ms ±   0.3 ms    [User: 8.3 ms, System: 1.4 ms]
  Range (min … max):     9.3 ms …  12.6 ms    282 runs

Since something that is no more present in the global namespace is removed, I've added a NEWS entry and a detailed changelog.

Importing `pickle` is now roughly 25% faster.

Importing the `re` module is no longer needed and
thus is no more implicitly exposed as `pickle.re`.
@picnixz picnixz force-pushed the perf/import/pickle-118761 branch from 0c5d01a to 6ce7785 Compare January 11, 2025 12:03
@picnixz picnixz added the performance Performance or resource usage label Jan 11, 2025
@picnixz picnixz changed the title gh-118761: improve import time of pickle gh-118761: improve import time for pickle Jan 11, 2025
Lib/pickle.py Outdated Show resolved Hide resolved
Co-authored-by: Adam Turner <[email protected]>
@picnixz
Copy link
Member Author

picnixz commented Jan 12, 2025

I'll merge this one tomorrow (and will check if removing isidentifier() improves a bit performances). I want to keep the first commit message as it indicates that re is now removed from the global namespace (it should never have been accessed from outside but we never know; maybe someone has been patching that attribute for whatever reason in their test suite).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting merge performance Performance or resource usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants