Repository navigation
Avoid copying the whole file when reading .pte and .ptd headers - #23561
HiroForYou wants to merge 10 commits into
Conversation
Reading the extended header with data[8:] copies the entire file, even though the header is at most 40 bytes. Slice only the bytes needed for the header instead.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/23561
Note: Links to docs will display an error until the docs builds have been completed. ❌ 5 New Failures, 11 Unrelated FailuresAs of commit 789d217 with merge base 0295293 ( NEW FAILURES - The following jobs have failed:
FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@pytorchbot label "release notes: none" |
|
@shoumikhin since you worked on this function in #23340, could you take a look when you get a chance? |
shoumikhin
left a comment
There was a problem hiding this comment.
Looks good, thanks. backends/mlx/pte_inspector.py makes the same whole-file copy when it reads the header, with pte_data[extended_header_offset:]. You could fix that here too, if you like.
|
@shoumikhin I applied the same fix to |
shoumikhin
left a comment
There was a problem hiding this comment.
Four notes, all small. The slicing change itself is right, and the .ptd test reads well.
Two of them are about the numbers in the description rather than the code.
The MLX row reads as a peak memory win, and for the Python process it is. But right after the header, parse_executorch_program() hands the whole file to _program_flatbuffer_to_json(), which writes it to a temp file and runs flatc, and flatc loads the file again in a child process. On a 384 MiB input that child peaks near 770 MiB either way, so the peak for the run as a whole does not really move. "One less full copy in Python" might describe it better.
The other is the two .pte rows. deserialize_pte_binary() calls _get_extended_header() on the whole file, so the saving on the outer call should be at least the saving on the inner one. The table has 455 ms for the header on its own and 198 ms for the full path. Allocator state in a fresh process could account for some of that, since the isolated benchmark has to fault in 384 MiB of fresh pages. Could you say which, or re-measure? bench.py covers the header row and the .ptd row but not the deserialize_pte_binary() row or the MLX one, so it is hard to check from the outside.
|
|
|
@shoumikhin, thanks for the second pass :). You were right about both numbers, and I updated the description:
I also merged |
|
@HiroForYou could you update the branch? Seems to be out-of-date. |
@nil-is-all done, the branch is up to date with main |
…the flat_tensor tests
shoumikhin
left a comment
There was a problem hiding this comment.
Looks good to me. The header read is bounded in one place now, deserialize() goes through the same helper instead of repeating it, and the empty __init__.py is a smaller fix than the rename. Thanks for measuring the import path question rather than taking my word for it.
Two things before this goes in.
The workflows on this push are still waiting to be approved and run, so the jobs that went red last time have not run again yet. Please get those green, since that was the whole reason for the previous round.
A few lines in the description no longer match the code:
- The part about dropping the ignore does not mention the
__init__.pythat makes dropping it safe. On its own the removal is what broke the jobs, so the two belong together. - It still says that reverting any of the three slices makes a test fail. There are two slice sites now, which matches the six cases you reported in the thread rather than nine.
- The test plan labels
105 passedas the number on main. That command gives 94 on main, and 105 is this branch.
|
@shoumikhin thanks for the review and all the suggestions :) I fixed the test count in the description, it's 105 on this branch and 94 on I also merged the latest |
|
@nil-is-all @shoumikhin I updated the branch with the latest changes from main since the previous CI runs had expired and the PR was blocked. The new workflows are waiting for approval. Once they pass, it's ready to merge. Thanks! |
Summary
_get_extended_header()inexir/_serialize/_program.pyandextension/flat_tensor/serialize/serialize.py, along withFlatTensorSerializer.deserialize(), read the extended header usingfrom_bytes(data[8:]).Slicing a
bytesobject copies the slice, sodata[8:]copies almost the entire file just to read a header that is at most 40 bytes.This change limits the slice to the header bytes (
8 : 8 + EXPECTED_LENGTH), which is allfrom_bytes()reads. Parsing results are unchanged.For
.ptefiles, the copy happens in_get_extended_header(), whichdeserialize_pte_binary()calls on the entire file. For.ptdfiles, it happened inFlatTensorSerializer.deserialize(), which had its own header read.FlatTensorSerializer.deserialize()andparse_executorch_program()inbackends/mlx/pte_inspector.pynow use_get_extended_header()instead of reading the header separately. This keeps the header parsing in one place and avoids duplicating the logic. One behavior change:deserialize()now raisesRuntimeError("Flat tensor header is invalid") instead ofValueErrorwhen the input is too short. I couldn't find any caller that relies on theValueError.I also removed
extension/flat_tensor/test/test_serialize.pyfrompytest.ini's--ignorelist so the.ptdregression test runs in the open-source CI job. The file shares its basename withdevtools/bundled_program/serialize/test/test_serialize.py. Since neither directory had an__init__.py, a repo-widepytestrun failed during collection withimport file mismatch. Adding an emptyextension/flat_tensor/test/__init__.pylets pytest import the test asflat_tensor.test.test_serialize, avoiding the clash.Test plan
Added regression tests that fail on main and pass with this change. Both wrap
from_bytes()and check that it receives exactlyEXPECTED_LENGTHbytes:exir/_serialize/test/test_program.py::test_get_extended_header_reads_only_header_bytescovers the.ptehelper, whichparse_executorch_program()now uses too.extension/flat_tensor/test/test_serialize.py::test_reads_only_header_bytescovers the.ptdhelper throughFlatTensorSerializer.deserialize().Reverting either slice to
data[8:],data[8:len(data)], orbytes(data)[8:]makes one of these tests fail.python -m pytest exir/_serialize/test/ extension/flat_tensor/test/ # 105 passed on this branch, 94 on main lintrunner exir/_serialize/_program.py extension/flat_tensor/serialize/serialize.py \ exir/_serialize/test/test_program.py extension/flat_tensor/test/test_serialize.py \ backends/mlx/pte_inspector.pyTested with a 384 MiB
.pte(6 xLinear(4096, 4096), XNNPACK) and a 384 MiB.ptd(same model exported withexternal_constants=True). Each operation was run 7 times in one process. The reported time is the median of the last 6 calls, then the median across 3 alternating before/after rounds. Memory is the increase in peak RSS of the Python process._get_extended_header()on the.ptedeserialize_pte_binary()FlatTensorSerializer().deserialize()parse_executorch_program()This avoids a 384 MiB copy in each path, saving around 300 ms in these measurements. Peak memory during full deserialization is unchanged (+384 MiB), since restoring the segments still copies the weights.
For the MLX inspector, this removes a full copy in the Python process. After reading the header,
parse_executorch_program()passes the entire file toflatcin a child process, which loads it again and peaks near 770 MiB. As a result, the overall peak memory for this path remains largely unchanged.In a fresh process, the first call is slower: the old slice also faults in 384 MiB of new pages, which the segment copies then reuse. So on a first call the header looks more expensive on its own than the saving on the full path, which is why the table uses the median of the repeated calls.
The parsed headers,
PTEFile,DataPayload, and MLX inspector output produce identical hashes before and after the change.Scripts to reproduce the measurements
Authored with assistance from Claude Code.