Repository navigation
M14: cargo xtask perf, performance budgets for large documents (gap 13) - #1
Harrison-Booth wants to merge 185 commits into
Conversation
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Benchmark execution and budgets xtask/src/main.rs, xtask/src/perf.rs, xtask/perf-budgets.json |
cargo xtask perf generates small, medium, and large PDFs and measures open, scroll, edit, incremental save, and full save in separate CLI processes. It compares elapsed time and readable peak memory with committed budgets, writes a JSON report, and fails when budgets are exceeded. |
Workflow and benchmark documentation .github/workflows/perf.yml, README.md, ROADMAP.md, docs/gaps.md, docs/roadmap.md, parity/acrobat-features.toml |
A workflow runs the benchmark nightly, on demand, and for selected pull request changes. The documentation records the measurements, budget scope, and platform limits. |
Application menus
| Layer / File(s) | Summary |
|---|---|
Menu construction and command handling crates/ui-egui/Cargo.toml, apps/pdfcraft/Cargo.toml, apps/pdfcraft/src/main.rs, crates/ui-egui/src/menus.rs, crates/ui-egui/src/commands.rs, crates/ui-egui/src/lib.rs, crates/ui-egui/src/chrome.rs, crates/ui-egui/src/widgets.rs |
The UI builds menus from command definitions and app state, and routes menu actions to command dispatch or menu-specific handling. App input processing integrates menu events and synchronizes native menu state. The custom popup menu and its helper are removed. |
Menu interaction test updates crates/ui-egui/tests/it/support/mod.rs, crates/ui-egui/tests/it/*.rs |
Shared test helpers open menus and find menu rows or similarly labeled controls outside menus. Integration tests use those helpers with the updated menu structure. |
Priority:
Estimated code review effort: 4 (Complex) | ~60 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant Xtask as xtask perf
participant CLI as pdfcraft CLI
participant Sampler as Linux peak-memory sampler
participant Budgets as perf-budgets.json
participant Report as JSON report
Xtask->>Budgets: Read time and memory limits
Xtask->>CLI: Run workload phase in child process
CLI-->>Xtask: Return phase result
Xtask->>Sampler: Sample process memory
Sampler-->>Xtask: Return peak memory when readable
Xtask->>Xtask: Compare measurements with limits
Xtask->>Report: Write measurement results
sequenceDiagram
participant NativeMenu as macOS native menu
participant App as PdfCraftApp
participant MenuCommand as run_menu_command
participant Dispatch as dispatch_command
NativeMenu->>App: Deliver menu click
App->>MenuCommand: Route click to shared command handler
MenuCommand->>Dispatch: Dispatch registered command
Suggested reviewers: aiongg, echelon
Merge Risk: 
fa43b
-
No identified issue blocks merging after normal checks.
Security architecture risk: 
fa43b
-
The benchmark introduces a temporary-directory ownership weakness that could let another local user redirect file writes when it runs on a shared machine. The menu changes retain execution-time permission and modal checks, while the new CI workflow limits repository permissions and does not persist checkout credentials.
Retained concerns
- Medium · security · inferred: The new benchmark treats a predictable temporary directory as trusted without exclusively creating it. On a shared temporary filesystem, another local user can precreate a writable pdfcraft-perf-PID directory containing symlinks for input.pdf, inc.pdf or steps.json. create_dir_all accepts the directory, and subsequent writes can follow those links and overwrite files writable by the benchmark user. Cleanup does not establish ownership or prevent the initial writes. The exposure is local and conditional on shared temporary-directory access, not demonstrated across isolated CI jobs.
Security review details
-
Security Blast Radius
- inferred — The temporary-directory concern exposes files writable by the account running cargo xtask perf when an attacker can prepare its shared temporary namespace. It does not independently grant repository write permissions or establish access to another isolated CI job.
Security Findings and Attack Paths
- inferred — A local attacker can anticipate the PID-derived directory name, precreate a writable directory, and place a symlink under a benchmark filename. The benchmark accepts that directory and writes through the link using the invoking user's authority. This attack path is introduced by the new benchmark and does not require attacker-controlled PDF content.
Trust Boundaries and Controls
- observed — The CI workflow uses pull_request rather than pull_request_target, requests only contents: read, disables persisted checkout credentials, and pins its actions to commit revisions. These controls limit workflow authority but do not establish ownership of the benchmark's local scratch directory.
- inferred — The inspected menu producers derive command identifiers from local menu construction and local UI or OS selections. PDF permissions influence command availability; the reviewed paths did not establish a PDF-content-to-menu-command injection route.
Resilience and Maintainability Implications
- observed — The benchmark refreshes the incremental-save input before each repetition, kills and waits for a phase exceeding 600 seconds, and attempts scratch cleanup on normal return or propagated errors. These recovery measures do not make scratch creation exclusive; forced process termination can also bypass drop-based cleanup.
- inferred — Queued native recent-file and document selections carry indices rather than stable identities. Collection changes before consumption can therefore retarget a still-valid index. Execution-time gates and bounds checks remain present, and no attacker-driven security consequence was established.
Hardening Proposals
- proposed — Create an unpredictable, exclusively owned temporary directory with restrictive permissions, and retain its ownership handle through child completion and cleanup. Reject pre-existing scratch paths rather than treating them as benchmark-owned state.
Pre-merge checks: 
-
Check name Status Explanation Resolution Description check The description follows the template's main idea. It covers what changes, why, how the numbers were chosen, verification, limits, and a next step. It does not follow the template's required headings. … Restructure the description to match the repository template. Add these sections and checklist items: 'What this changes' (as a heading), 'How you verified it' with the checklist state for cargo fmt --check, clippy with-D warnings, tes…Docstring Coverage Docstring coverage is 48.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 103 functions across 19 files. (9 skipped… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title names the main change: a new cargo xtask perfcommand that enforces performance budgets for large documents, tied to gap 13. The changeset adds that command, its budget file, a CI workflow…Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: a new
cargo xtask perfcommand that enforces performance budgets for large documents, tied to gap 13. The changeset adds that command, its budget file, a CI workflow, and the related docs and parity updates, so the title matches the primary change. It is specific and concise.
-
Full details: Description check
-
Explanation
The description follows the template's main idea. It covers what changes, why, how the numbers were chosen, verification, limits, and a next step. It does not follow the template's required headings. It omits the 'What this changes' and 'How you verified it' headings, the checklist items for fmt, clippy, and tests, the Never-crash section, the Provenance section, the Security section, the Parity and reach checklist, and 'Anything a reviewer should look at first'. It also says the author did not run cargo locally, and it does not mention the xtask layers or assets checks. The Provenance and Security checklists are relevant to a new xtask and a CI workflow that adds no network access, so they need an answer from the author. The description is mostly complete on content but misses several template sections.
Resolution
Restructure the description to match the repository template. Add these sections and checklist items: 'What this changes' (as a heading), 'How you verified it' with the checklist state for
cargo fmt --check, clippy with-D warnings, tests, andcargo xtask layersandcargo xtask assets(the author touched assets throughATTRIBUTION.toml-related docs, so state whether these pass). Add a 'Never-crash' section that confirms the newxtask/src/perf.rscode has nounwrap/expect/panic!outside tests, or explains why. Add a 'Provenance' section that confirms no Adobe assets, no font files, and no GPL code. Add a 'Security' section that confirms no new outbound network access and states whether the new CI workflow touches any opt-in or token-authenticated surface. Add a 'Parity and reach' section that answers theparity/acrobat-features.tomlitem and states whether theautomationtool applies. Add 'Anything a reviewer should look at first' with the budget-choice judgement call and the Linux-only memory reading. Mark each checklist item as checked or explain why it is not applicable.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 48.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 103 functions across 19 files. (9 skipped: 9 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
2883a66 to
7859fb5
Compare
… not a skipped budget
Use the fixed release for GHSA-vc25-qg58-w57c.
refactor: shared module layout (craft-contrib#20)
Export > Image spent its time in the PNG, JPEG and TIFF encoders, one page after another: on a 55-page manual at 150 dpi, drawing every page took about 0.34 s and encoding it 1.1 s (PNG) to 2.0 s (JPEG). Exporter::images exports a list of pages, drawing and encoding several at once on worker threads, each with its own PageRenderer (as automation's extract_parallel does), and hands the files to a sink in page order on the calling thread. Workers draw at most one page ahead of the sink and fewer run when 1 GiB can't hold that many of the largest page. The first page or sink error stops the export with every earlier page handed over, as before. A page panic becomes that page's error, and a panicking sink stops the workers. One page, one core or wasm keeps the per-page loop. Export > Image and the doc_export_images tool use it. Files are byte-identical. One core is left for the interface (at most 8 workers). With examples/export_bench, on a Ryzen with 16 threads (shared), mean of two medians of 21 runs: PNG 150 dpi 1874 -> 340 ms (-82%) JPEG 150 dpi 2106 -> 340 ms (-84%) TIFF 150 dpi 2032 -> 375 ms (-82%) PNG 300 dpi 3804 -> 590 ms (-84%) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
perf(engine): export page images on worker threads (about 80% less wall time)
…urls Packaging: fetch OCR models from the ONNX URLs (legacy RTEN URLs return 403)
What
cargo xtask perfadds performance budgets for large documents (gap 13,misc.performance-budgets).pdfcraft-clionce per phase, in its own process: open, scroll (render the first 10 pages at 72 dpi), edit (set a title, rotate a page), incremental save, full save.VmHWM), keeps the fastest time and highest peak of--runs(default 3), writes JSON totarget/perf/results.json, and compares against the committedxtask/perf-budgets.json. It exits non-zero when a budget is exceeded or a phase has no budget..github/workflows/perf.ymlruns it nightly, on demand, and on changes to the benchmark. It is not a required check.docs/gaps.mdgap 13,docs/roadmap.md,ROADMAP.md, the README command list and themisc.performance-budgetsparity entry (nowpartial).Why
Gap 13 lists budgets as missing; memory and timing so far were one-off manual measurements. This makes them repeatable and checkable.
How the numbers were chosen
Measured on a GitHub
ubuntu-latestrunner: the slowestlargephase took 1.3 s and peaked at 347 MiB; open was 0.4 s at 145 MiB. Budgets are at least eight times the measured time and 2.5 to 4 times the measured peak (for examplelargesaves: 20 s, 1000 MiB). They catch an order-of-magnitude regression such as an extra copy of the file, not a few percent. Times for a phase include the open, since each phase is its own process.Verification
lopdf), filler size, and phase scripts.Not verified / limits
small) read low; it only bounds the larger sizes.Proposed next step
After a few nightly runs, tighten the budgets and make the job a required check, probably only for changes touching the engine, render or cos crates.
Summary by CodeRabbit