Persist seed-mismatch re-packs so the packed cache converges after an XNNPACK upgrade - #22183
Persist seed-mismatch re-packs so the packed cache converges after an XNNPACK upgrade#22183doggeral wants to merge 1 commit into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22183
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 2 Unrelated FailuresAs of commit bad763c with merge base 6e6477d ( NEW FAILURE - The following job has failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following job failed but was 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. |
|
@doggeral has exported this pull request. If you are a Meta employee, you can view the originating Diff in D117550758. |
This PR needs a
|
… XNNPACK upgrade Summary: After an XNNPACK upgrade the per-ukernel seed changes, so every entry loaded from the packed weight cache misses `look_up` (seed mismatch) and XNNPACK re-packs it. `reserve_space` routes all re-packs to heap whenever `loaded_from_disk_` is set, and `save_packed_index` only persists when new mmap regions were added — so the refreshed packing is never written back. Every launch then reloads the same stale file, re-packs all weights into anonymous (dirty) heap, and, when backgrounded, can be OOM/jetsam-killed. The cache never converges. Fix: track a per-`look_up` `last_lookup_seed_mismatch_` flag, set when a named entry is present but its cached seed differs from the current ukernel's seed. `reserve_space` keeps unnamed constants and incidental warm re-packs on heap as before, but routes a seed-mismatch re-pack to the file-backed path. This lets `save_packed_index` persist the refreshed seed, so the next launch hits and the cache converges after a single rebuild, with the re-pack landing in reclaimable file-backed pages instead of anonymous dirty memory. Note: the old packed bytes of re-packed entries remain as orphans, so the file grows ~once per XNNPACK upgrade (see the `file_bytes` logging in `save_packed_index`); orphan GC/compaction is a follow-up. --- **V2 — addressing review comments** **1. "For this 'miss' — can you ever get here with `loaded_from_disk_ == true`?"** Yes, and that is exactly why the `loaded_from_disk_` clause has to stay. Two ways in: - A named entry that is simply absent from the loaded index (a weight the cached PTE never had) misses `look_up` with both flags false. - More importantly, per D111354375 cause pytorch#3, XNNPACK re-packs some weights for a different runtime context by calling `reserve_space` with **no preceding `look_up` at all**. That second case exposed a bug in V1: with no `look_up` to reset it, `last_lookup_seed_mismatch_` stayed `true` from an earlier lookup and would route such a re-pack to the file, re-growing the cache on every load — the very regression D111354375 fixed. `reserve_space` now **consumes** the flag (reads it, then clears it), so only the re-pack that directly follows a seed-mismatch `look_up` takes the file-backed path. **2. Writing the tests surfaced a pre-existing bug that made V1 a no-op** `load_packed_cache` takes `flock(LOCK_SH)`, `mmap`s the file `MAP_SHARED`, then `close(fd)`. The live mapping keeps the open file description — and therefore the `flock` — alive, so `close()` never drops the lock. The `open_locked(..., O_RDWR)` that follows in `initialize_for_runtime` then always fails with `EWOULDBLOCK`, leaving `packed_file_fd_ == -1`. Confirmed: ``` LOCK_SH -> mmap -> close -> LOCK_EX => EWOULDBLOCK LOCK_SH -> close -> LOCK_EX => OK ``` With `packed_file_fd_ == -1`, `reserve_space`'s file-backed branch is unreachable and `save_packed_index` early-returns, so **every write path after a warm load was dead** — including the seed-mismatch re-pack this diff adds. The `flock(...) failed (errno=11)` line is present in the logs of every existing cache test, right after "Loaded packed weight cache"; nothing asserted on it. Fixed with an explicit `flock(fd, LOCK_UN)` before the `close(fd)`, which preserves exactly the lock scope the original `close()` placement intended. **3. Tests** Two new tests in `test_xnn_weights_cache.cpp`: - `SeedMismatch_RepackIsPersistedToFile` — tamper the persisted seed to simulate the upgrade, then assert the next launch writes the refreshed seed back and appends the re-packed bytes to the file (i.e. it did not land on heap), with unchanged inference output. - `SeedMismatch_CacheConvergesAfterOneRebuild` — assert exactly one launch pays for the rebuild and the three launches after it are pure hits: no file growth, seed stable, output stable. Both fail on V1's routing logic and pass with the fix. Also factored the duplicated footer/seed-field walking in the existing tests into `read_first_entry_seed` / `write_first_entry_seed` helpers (the old inline corruption write was host-endian; the helper is little-endian like the format). Differential Revision: D117550758
4c27859 to
bad763c
Compare
Summary:
After an XNNPACK upgrade the per-ukernel seed changes, so every entry loaded
from the packed weight cache misses
look_up(seed mismatch) and XNNPACKre-packs it.
reserve_spaceroutes all re-packs to heap wheneverloaded_from_disk_is set, andsave_packed_indexonly persists when new mmapregions were added — so the refreshed packing is never written back. Every
launch then reloads the same stale file, re-packs all weights into anonymous
(dirty) heap, and, when backgrounded, can be OOM/jetsam-killed. The cache
never converges.
Fix: track a per-
look_uplast_lookup_seed_mismatch_flag, set when a namedentry is present but its cached seed differs from the current ukernel's seed.
reserve_spacekeeps unnamed constants and incidental warm re-packs on heap asbefore, but routes a seed-mismatch re-pack to the file-backed path. This lets
save_packed_indexpersist the refreshed seed, so the next launch hits and thecache converges after a single rebuild, with the re-pack landing in
reclaimable file-backed pages instead of anonymous dirty memory.
Note: the old packed bytes of re-packed entries remain as orphans, so the
file grows ~once per XNNPACK upgrade (see the
file_byteslogging insave_packed_index); orphan GC/compaction is a follow-up.V2 — addressing review comments
1. "For this 'miss' — can you ever get here with
loaded_from_disk_ == true?"Yes, and that is exactly why the
loaded_from_disk_clause has to stay. Twoways in:
cached PTE never had) misses
look_upwith both flags false.a different runtime context by calling
reserve_spacewith no precedinglook_upat all.That second case exposed a bug in V1: with no
look_upto reset it,last_lookup_seed_mismatch_stayedtruefrom an earlier lookup and wouldroute such a re-pack to the file, re-growing the cache on every load — the very
regression D111354375 fixed.
reserve_spacenow consumes the flag(reads it, then clears it), so only the re-pack that directly follows a
seed-mismatch
look_uptakes the file-backed path.2. Writing the tests surfaced a pre-existing bug that made V1 a no-op
load_packed_cachetakesflock(LOCK_SH),mmaps the fileMAP_SHARED, thenclose(fd). The live mapping keeps the open file description — and thereforethe
flock— alive, soclose()never drops the lock. Theopen_locked(..., O_RDWR)that follows ininitialize_for_runtimethen always fails withEWOULDBLOCK, leavingpacked_file_fd_ == -1. Confirmed:With
packed_file_fd_ == -1,reserve_space's file-backed branch isunreachable and
save_packed_indexearly-returns, so every write path aftera warm load was dead — including the seed-mismatch re-pack this diff adds.
The
flock(...) failed (errno=11)line is present in the logs of everyexisting cache test, right after "Loaded packed weight cache"; nothing asserted
on it.
Fixed with an explicit
flock(fd, LOCK_UN)before theclose(fd), whichpreserves exactly the lock scope the original
close()placement intended.3. Tests
Two new tests in
test_xnn_weights_cache.cpp:SeedMismatch_RepackIsPersistedToFile— tamper the persisted seed tosimulate the upgrade, then assert the next launch writes the refreshed seed
back and appends the re-packed bytes to the file (i.e. it did not land on
heap), with unchanged inference output.
SeedMismatch_CacheConvergesAfterOneRebuild— assert exactly one launch paysfor the rebuild and the three launches after it are pure hits: no file
growth, seed stable, output stable.
Both fail on V1's routing logic and pass with the fix. Also factored the
duplicated footer/seed-field walking in the existing tests into
read_first_entry_seed/write_first_entry_seedhelpers (the old inlinecorruption write was host-endian; the helper is little-endian like the format).
Differential Revision: D117550758