Skip to content

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

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

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

Conversation

@huntharo

@huntharo huntharo commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

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 after rebasing onto upstream 9b18b27 and repairing CI portability failures: cargo xtask lint and RUST_TEST_THREADS=1 cargo xtask test pass locally on macOS arm64 with the default temporary-directory settings: 106 app + 174 core + 4 scanner integration + 3 xtask tests (287 total), with eight opt-in tests ignored. All original and expanded regular budgets pass. Windows-target core Clippy (--target x86_64-pc-windows-msvc --all-targets --all-features -- -D warnings) also passes; native Windows execution remains for CI. The first parallel local test run aborted in the macOS notify-rs/test-scheduler thread machinery; the full serial run passes.

The CI repair changes only test fixtures and test-only compilation: the picker assertion expects the canonical path (macOS symlinks/Windows short names), the remote-root fixture uses a guaranteed-missing native absolute path, and the two shell-test helpers compile only on Unix. The hardlink integration test checks deduplicated/non-deduplicated totals on every platform, checks live preview contents on Unix, and checks the absence of a preview for the non-streaming Windows fallback. Both remaining Windows jobs had failed by unwrapping that absent preview, after their byte/file assertions had passed. Production behavior and removal guards are unchanged by these repairs. The rebase preserves upstream’s read-only SSH plan and measured mark bytes in file-count mode, with refresh coverage for both metrics. The timings above remain the original measured comparison against main 6c8d4ce; they were not remeasured during conflict or CI repair. 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 Permanent deletion empties filesystems mounted inside a marked directory #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

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.
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