perf(engine): restore leaves in walk order, not tree order - #455
Conversation
topoSort ordered the whole snapshot parent-before-child. Only the entries that are parents need that. Everything else is a leaf, and a leaf may be written at any point once the interior of the tree exists. Ordering the leaves topologically grouped them by directory, which bears no relation to how they are laid out in packfiles, so the write phase read scattered across every pack. Measured by tracing which pack each catalog resolution lands in, on a 51-pack tree: 55.5% pack-cache misses in the write phase against 0.6% in the metadata phase, which already reads in tree-walk order. So emit the interior first, parent-before-child, then the leaves in walk order -- the order backup wrote them, which is the order they sit in packs. Modelling PackStore's four-pack body cache over the traces, whole-pack transfers for a full restore fall from 51413 to 6726, with no extra requests: these are reads that were happening anyway, in a better order. Interior membership is derived from the data rather than from Type. A regular file is a leaf in every source model here -- local and sftp take the parent from path.Dir, onedrive from ParentReference, gdrive from the Drive parents field, all of which name folders -- but a snapshot is data read off a store, and an entry claiming a file as its parent must still be ordered after it rather than trusted not to exist. collectMetadata now reports walk order alongside the entries. Results are placed by index rather than appended as they land, so the fetch concurrency cannot decide the write order. Part of RFC 0023 §5, and independent of the catalog bound it exists to enable.
rmanibus
added
area/core
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Summary
restoreOrderreplacestopoSort: the interior of the tree is ordered parent-before-child, and the leaves follow in tree-walk ordercollectMetadatareports walk order alongside the entries, placing results by index so fetch concurrency cannot decide the write orderTypetopoSortordered the whole snapshot topologically. Only entries that areparents need that; everything else is a leaf and may be written at any point once
the interior exists. Ordering leaves topologically grouped them by directory,
which bears no relation to packfile layout.
Part of RFC 0023 §5.
Measurement
Traced which pack each catalog resolution lands in, on a 50,000-file tree
configured to produce 51 packs, then modelled
PackStore's four-pack body cacheover the traces.
mainmain+ #452A 7.6x reduction in pack transfers, and unlike #452 it costs no extra requests —
these are reads that were happening anyway, in a better order. The two compose:
together they are 30x fewer transfers than
main.Pack-cache miss rate for the write phase drops from 55.47% to 0.74%, which now
matches the metadata phase's 0.62% — the asymmetry between the two phases is
gone.
On deriving interior membership from data
RFC 0023 §5 left an open question: is a regular file ever a parent? Checking the
source models —
localandsftptake the parent frompath.Dir,onedrivefrom
ParentReference.ID,gdrivefrom the Driveparentsfield, and gdrivetypes a folder purely by mime type with no shortcut handling — the answer is no.
The implementation does not rely on that. A snapshot is data read off a store,
and
collectMetaPathsalready guards against unresolvable parents and cycles, soan entry naming a file as its parent is ordered after it rather than trusted not
to exist. That costs nothing: in practice the interior set is the folders.
Note on a measurement method
An earlier version of this used
-debugoutput line counts. Those are notreliable: when a progress reporter is active,
SafeLogWriter.Writeroutes topw.Log(), a bounded buffer that drops under load — a restore issuing 109,597lookups produced 9,097 log lines. The figures above come from the trace
instrumentation instead, which writes synchronously under a mutex. I have posted
a correction on #452, whose description carried a figure derived the unreliable
way.
Verification
Both pass; lint reports 0 issues. A full restore of the 50,000-file fixture
produces all 50,000 files.
Ordering tests cover parent-before-child, leaves keeping walk order, a file named
as a parent, cycle termination, unknown parents, entries absent from walk order,
and the empty snapshot.
restore_parallel_test.goadditionally asserts walk orderis complete and gap-free, which is what the index-placement change guarantees.