-
Notifications
You must be signed in to change notification settings - Fork 73
Stream package extraction to bound peak memory usage #410
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
Open
joaogabriel15
wants to merge
1
commit into
python:main
Choose a base branch
from
joaogabriel15:fix/stream-package-extraction
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+45
−2
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How badly does it affect the maximum memory usage if we make this use 10MB instead of 1MB? I'd like our standard Python distributions to still only use one call per file, and the biggest file we include is (currently) around 7MB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I measured this on Windows 11 with CPython 3.14.3, using the PR's actual extract_package function and changing only the copyfileobj buffer size in an in-memory benchmark variant.
For synthetic ZIP_DEFLATED archives containing one member each, the median tracemalloc peak across five extractions was:
Archive creation was outside the measured region. Each extraction used a fresh destination, and I verified the extracted contents afterward. The payload was a repeating bytes(range(256)) pattern, so these are highly compressible synthetic cases, not measurements of standard runtime packages. These figures measure peak traced Python allocations during extraction, not total process RSS; they include zipfile/decompression allocations as well as the copy buffer.
The 10 MiB buffer therefore costs more than just the extra 9 MiB of buffer capacity, but still bounds memory for oversized members. A 7 MiB file fits in one data-bearing read/write with that buffer (copyfileobj also performs a final EOF read).
Punisheroot has also posted complementary measurements using real 3.12/3.13/3.14 packages here: #409 (comment) . Their reported results likewise support 10 MiB as a compromise: single data-bearing reads for normal distribution files, with bounded memory for oversized files.
Based on these results, 10 MiB looks reasonable to me for the goal you described.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up with measurements using real official runtime packages rather than synthetic files.
I tested the AMD64 ZIP distributions for Python 3.12.10, 3.13.15, and 3.14.7 from the official Windows install-manager index, verifying each download against its published SHA-256. Together they contain 10,241 files and 376,099,043 uncompressed bytes; the largest member is 6,945,272 bytes.
Setup: Windows 11 (build 26200), CPython 3.14.3 AMD64. Each run used a fresh process and destination and extracted all three packages sequentially using this PR's extract_package function. The variants changed only the copy operation: original zf.read(), 1 MiB copyfileobj, or 10 MiB copyfileobj. There was one warm-up per variant, then five measured runs per variant in rotating order.
Moving from 1 MiB to 10 MiB increased the median traced peak by 14.38 MiB and peak working set by 9.88 MiB. Compared with the original, 10 MiB reduced traced allocations somewhat, but the process working-set peak was essentially unchanged on these normal-sized packages.
A separate instrumented extraction confirmed that all 10,164 non-empty files completed in exactly one data-bearing read with 10 MiB. There were 20,405 reads including EOF reads across all 10,241 files. This diagnostic run was excluded from the measurements above.
SHA-256 output manifests, including relative paths and per-file digests, matched across all 18 benchmark executions and the separate read-count run.
Limitations: downloads, imports, integrity hashing, and cleanup were excluded from extraction timing. tracemalloc was enabled during all timed runs, so timings include its overhead. Windows working-set peaks were read with GetProcessMemoryInfo immediately after extraction, before hashing, and include the interpreter/tracing machinery. OS caches were not flushed and background activity was not controlled. The timing ranges overlap substantially, so I would not claim a reliable speed improvement from this run.
This confirms that 10 MiB meets the single-data-read goal for these distributions, at the cost of giving up much of the normal-package memory saving of 1 MiB. Oversized members remain a separate case, covered by the earlier synthetic measurements. No production code has been changed for this benchmark.