Add opt-in interval first-fit memory planner - #22508
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22508
Note: Links to docs will display an error until the docs builds have been completed.
|
meta-cla
Bot
added
the
CLA Signed
|
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a66bd3b1dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if lower_bound is None or greedy_result.bufsizes == lower_bound: | ||
| return greedy_result |
There was a problem hiding this comment.
When the declared Set[TensorSpec] input contains equal-size tensors and greedy already reaches the lower bound, this return bypasses _stable_spec_order and preserves greedy's set-iteration-dependent tie ordering. Because greedy inserts equal-size specs without a secondary key, equivalent planning runs in different processes can assign those specs different offsets and produce nondeterministic serialized programs; the added determinism test only repeats the same set instance and therefore cannot detect this. Normalize the baseline's ordering or use the stable candidate layout before returning this tie.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding, thanks. Addressed in 3e3016c: the opt-in conditional path now computes graph-stable order once and passes that ordered iterable to upstream greedy before lower-bound, hard-cap, and soft-benefit returns. The test now compares adversarial forward/reverse set iterators instead of repeating one set instance. Default greedy and unsupported-alias fallback remain unchanged. Validation and model reruns for this new head are still pending.
There was a problem hiding this comment.
Fixed in 3e3016c and refined in 4e5a390. The opt-in conditional baseline is now ordered deterministically by (allocated size, lifetime start, lifetime end, memory ID), consulting graph order only for exact key collisions; the default greedy path is unchanged. The regression test uses adversarial forward/reverse Set iteration orders rather than repeating one Set instance. Final validation on 4e5a390 includes 10,000 randomized cases with zero failures and exact deterministic replay on all 10 frozen holdout models.
This PR needs a
|
2 participants
Summary
Current ExecuTorch
greedy()already sorts allocations by decreasing size, but its shared-object placement can retain holes when a lower aligned legal offset exists. This adds an opt-in interval first-fit candidate and a bounded conditional portfolio that returns the smaller of current upstream greedy and the candidate.Default greedy behavior is unchanged. The opt-in policy uses per-memory-ID aligned peak-live lower bounds, inclusive lifetime overlap, hard and soft work limits, and whole-planning-unit fallback for unsupported storage-backed aliases. Equal-size baseline ties are deterministic: allocation-relevant keys are used first, with graph order only for exact key collisions.
Correctness and safety
Current-head evidence
Measurements are pinned to upstream
457a2a8b9f7d103765d73752c5d2efc6b2e8c8bc, PR head4e5a3906f456120dffd2ec31ea097b50dde17303, and cumulative patch SHA-2567ad25503ce5d048defd193be39ca08ce38d477f100c3b1d53ec3700d9a91241b.All ten layouts were valid and deterministic, and all ten serialized upstream-versus-candidate runtime replays were exact. Eight selected layouts reached the sound lower bound. Median-across-case planner times were 17.104 ms for upstream, 45.710 ms for candidate-only, 62.828 ms for always-both, and 65.253 ms for conditional execution; the largest conditional median was 1,077.919 ms on SwinV2-S. Timing is local export-time evidence, not production latency evidence.
ResNet152 and Wide-ResNet101-2 have exact upstream-versus-candidate portable-runtime equality but nonconfirmatory eager diagnostics. Both remain in the results. Emformer and MobileBERT no-gain controls stayed below the frozen 100 ms added-time blocker.
The external benchmark repository contains every model row, exact commands, four-mode timings, retained failures, provenance, and checksums. Immutable snapshot: 07ca869a88c27105b0ae322f90bca259bd95cd32.
Validation and known environment failure
Changed-file lint and diff checks pass. The memory-planning test file reports 51 passes and one local failure because the compiled
llama.sdpa_with_kv_cachecustom-op library is absent; pristine pinned upstream fails the same test identically.Scope and review order
The PR changes only
exir/memory_planning.pyandexir/tests/test_memory_planning.py. No benchmark driver, corpus, generated result, or agent report is included.Review
exir/memory_planning.pyfirst for interval placement, lower-bound soundness, work exits, deterministic baseline ordering, alias fallback, and memory-object IDs. Then review the seven focused tests.AI disclosure
This patch and evaluation were produced with OpenAI Codex assistance and require maintainer review.