Skip to content

Fix quadratic marking, path resolution, and removal review - #9

Closed
huntharo wants to merge 1 commit into
mainfrom
fix/main-mark-performance-20261014
Closed

huntharo wants to merge 1 commit into
mainfrom
fix/main-mark-performance-20261014

Conversation

@huntharo

@huntharo huntharo commented Oct 14, 2026 •

Copy link
Copy Markdown
Owner

Superseded by tobi/disktree#105. The 12 development commits have been consolidated into 11ff1d98ab5daf357c5a0319ad4943a69e6bb164. This fork PR is retained as the historical development/review record.

Thanks for disktree. I’m looking at programmatic/MCP cleanup workflows that can queue many unrelated paths for review. Main repeatedly scans those queues during marking and rendering; this PR bounds that work in one main-based change, consolidated into one commit covering path resolution, queue mutation, and Review status lookup.

Development and review took place in huntharo/disktree#9, which this upstream PR supersedes. All fixes and evidence are consolidated into one commit based directly on main.

A mark is one file or folder queued for removal. A folder containing a million files is still one mark. A queue of 1,024 independently marked paths is a large-queue stress case, not an established frequent manual workload. This PR improves the existing state/render paths; it does not add an MCP endpoint.

The cost is paid at different points:

  • Building a queue: repeated state-level additions, before drawing. Ancestor and descendant searches currently scan all prior marks.
  • Marking a parent: one marking action can compact the mark vector once per absorbed child.
  • Redrawing Explore or Review: marking/unmarking, selection, hover and opening Review can trigger rendering. The plan processes the queue again; Explore also resolves marked paths, and Review searches blocked/covered lists for each row.
  • Receiving a scan result or changing the metric: Marks::refresh resolves the marked paths again. This refresh is not part of every hover or marking action.

The previously reported 6.60 seconds measured four forced Explore draws/eight prepares, with a large queue already present. It was not one scan, one click, or the time to select 1,024 items. The input measurements below have a different, explicit denominator.

Already queued paths One Space input: main → combined One hover input: main → combined Open Review with C: main → combined
1 3.79 → 3.76 ms 3.94 → 3.87 ms 3.47 → 3.43 ms
8 5.14 → 5.01 ms 5.28 → 5.14 ms 5.82 → 5.74 ms
128 41.54 → 16.13 ms 40.85 → 16.26 ms 62.99 → 38.19 ms
1,024 1,584.73 → 96.49 ms 1,575.83 → 97.78 ms 1,786.18 → 300.71 ms

These are medians of three input repetitions in the real GPUI window harness on a fixed, real temporary tree: 1,025 sibling folders with one file each, a 1,400×900 window, and both panels visible. Space adds one more folder; hover/Review then run with that extra mark. Each timed input causes two or three renders in the harness and asserts zero mark refreshes/rescans. Main was replayed immediately before the combined implementation on macOS arm64/Rust 1.97.1 with the repository test profile. These are observed harness timings, not release-app per-click guarantees. Small queues were already fast; no substantial small-queue speedup is claimed.

The implementation changes six paths:

Path Resulting bound and behavior
Removal nesting Sort precomputed platform guard keys, then check the last retained ancestor: O(M log M) path comparisons plus M prefix checks. Output preserves its path ordering.
Mark refresh One lazy sibling index per resolution batch: expected O(S + M·D), where S is indexed sibling entries (S ≤ scanned nodes) and D is path depth.
Explore marked crumbs Share the resolved set across unchanged frames. Mark/tree/root/metric changes and refresh invalidate it; a weak tree identity avoids retaining an old scan.
Disjoint queue construction Ordered exact-path index plus ancestor/range queries: O(M·D·log M) index work for M additions, rather than O(M²) mark visits. Both file and folder queues are covered.
Parent absorption One stable compaction and indexed removal of K descendants: O(M + K log M), plus the ancestor lookup for that action.
Review row status Borrow covered/blocked indexes from the newly computed plan: expected O(B + C + L) status work for B blocked, C covered and L displayed rows (L ≤ 1,200).

These bounds have additional path/name-length costs. Exact mark membership changes from expected hash O(1) to ordered O(log M); no fields are added to scan nodes. Mark order, existing path/component/case comparisons, exact non-UTF-8 paths, first blocked reason, absorbed-count notices and cached-crumb invalidation are tested. Single-item unmarking still compacts the ordered vector; multiple separately invoked parent edits can each require a compaction.

The expansion has its own red/green evidence. The “before” below is pre-expansion #9, with the original path-resolution/removal-plan fixes already included, not a fresh comparison against main:

Added workload (1,024-item setting) Logical work before → combined Median wall before → combined
Queue all 1,024 files, no drawing 1,048,576 → 9,216 176.13 → 6.41 ms
One parent-marking action over 1,024 descendants and one neighbour 528,899 → 2,058 15.60 → 0.47 ms
Blocked Review, four draws/eight renders 524,800 → 3,072 per render 1,003.88 → 883.69 ms total
Covered Review, four draws/eight renders 525,824 → 3,074 per render 1,110.66 → 1,009.63 ms total

The blocked fixture queues paths outside the scanned root. The covered fixture supplies a raw overlapping parent/children queue, which normal manual marking would absorb. These fixtures plan and render only; they attempt no deletion.

Mutation units count visits/index requests, not internal B-tree comparisons; the logarithmic factor is included in the bounds above. Review units include index construction and row queries. The regular budgets grow 128→1,024 entries, require at most 16× work and enforce absolute caps. File insertion, folder insertion, parent absorption and both Review status cases fail before their fixes and pass afterward. Review also passes at 8,192 items, with 10,592 status units per render and the 1,200-row cap preserved. The tests drive actual state transitions or window rendering, so a faster helper alone cannot satisfy them.

Review’s total wall improvement is smaller than its status-lookup reduction: the screen still lays out up to 1,200 rows and evaluates guards/volume attribution for the queue. Plan computation remains per render, and the removal worker still rereads mounts/checks links immediately before deletion. Only display data is reused across renders; no removal authorization is cached. Future MCP bulk control should submit additions within one UI transaction to avoid a redraw per tool call.

Combined evidence, historical snapshots, raw samples, complexity details and reproduction commands. Earlier measurements are retained and labelled as historical stages. The deeper breadcrumb, mount-topology and unusual classification cases from the broader audit remain separate follow-ups. The PR has no dependency on the APFS feature branch.

Validation: cargo xtask lint passes; cargo xtask test passes 88 app + 150 core + 3 xtask tests (241 total). Two pre-existing ignored core tests and the opt-in timing test remain ignored by the normal suite; the timing test passes explicitly. All original and expanded budgets pass. Local validation is macOS arm64; other platforms remain for existing CI. Scans and destructive tests use private temporary fixtures.

Historical red/green snapshots and reproduction

The IDs below record pre-consolidation development snapshots from the fork PR; they are not separate commits in this upstream branch. The linked evidence explains how to fetch each historical snapshot.

  • 5a3c92e: original main algorithms plus probes; original refresh/window/removal-nesting budgets fail.
  • 1206fda: original path-resolution/removal-plan fixes.
  • 57d4efd: new queue/parent/Review budgets fail on pre-expansion Fix quadratic marking, path resolution, and removal review #9.
  • e88b9e1: indexed marking and one-pass absorption.
  • b71718f: indexed Review status from each freshly computed plan.
  • 1c36fae: folder insertion budget, also demonstrated failing against the old algorithm.
  • d886865: preserve the original ancestor choice for raw path spellings with redundant separators.

To reproduce the expanded red stage in a disposable checkout:

git fetch https://github.com/huntharo/disktree.git \
  57d4efde372e7f08e0a2b91a906ff3b870abaddc
git switch --detach FETCH_HEAD
cargo test -p disktree-app budget -- --nocapture --test-threads=1

At the final head:

cargo xtask lint
cargo xtask test
cargo test --workspace budget -- --nocapture --test-threads=1
cargo test -p disktree-app marked_interaction_timings -- \
  --ignored --nocapture --test-threads=1

@huntharo huntharo changed the title Fix quadratic marked-path lookup and removal planning Fix quadratic marking, path resolution, and removal review Oct 14, 2026
Batch marked-path resolution and cache display crumbs across unchanged frames. Bound removal nesting, index queue mutations, absorb descendants in one pass, and index Review status from each fresh plan.

Include deterministic state/window work budgets, path and invalidation regression coverage, and measured red/green evidence. Removal authorization remains freshly revalidated before deletion.
@huntharo

Copy link
Copy Markdown
Owner Author

Superseded by upstream PR tobi#105. Consolidated all 12 development commits into 11ff1d9; runtime source is unchanged from the reviewed head. Lint and all 241 enabled tests pass after consolidation. Historical red/green evidence remains linked in the upstream PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant