Repository navigation
perf(check): read only the parts of artifact files the checkers need - #2235
Conversation
The output checkers and lib_symbols read every artifact file whole to parse its ELF header, imports and dynamic symbols, so a full-catalog sweep read every byte of every output. Map them read-only with MADV_RANDOM: on a cold sweep of 39 GiB of artifacts, 123 s / 19 GiB from disk becomes 26 s / 1.8 GiB, with identical results (build-servers#210). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters: 1
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configuration
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. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main performance change. It follows the repository's Conventional Commit format. Description check The description explains the change, motivation, measured results, impact, and testing status. It omits the template Checklist and does not include command output, but it is otherwise mostly complete. 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: Docstring Coverage
-
Explanation
Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files. (1 skipped: 1 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
A rabbit checks each file with care
No whole-file heap is needed there
A shebang marks the script’s first line
The cached reader keeps its place in time
Then hops away, its checks in line
Comment @coderabbitai help to get the list of available commands.
|
Approved: no findings at Nothing to do. Clobber: none seen. |
There was a problem hiding this comment.
correctness
minimal-review — approved
Verdict APPROVE — No findings.
security
minimal-review — advisory review
Verdict COMMENT — 1 finding, 1 inline.
supply-chain
minimal-review — approved
Verdict APPROVE — No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
correctness
minimal-review — advisory review
Verdict COMMENT — 1 finding, 1 inline.
security
minimal-review — approved
Verdict APPROVE — No findings.
supply-chain
minimal-review — approved
Verdict APPROVE — No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @bryan-minimal closed the last one. The review of f15fa4f51468 withheld this approval on the threads open then.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A mapped artifact turned a file truncated under the check into SIGBUS for the whole process. These files are hardlinked into build sandboxes, and a build writing through the link (cp or > onto it) truncates the shared inode, so "nothing rewrites them in place" was assumed, not enforced. object::read::ReadCache reads only the ranges the parsers ask for, so a truncated file is one short read and one parse error. With POSIX_FADV_RANDOM it keeps the cold-sweep win: 24-27 s / 2.0 GiB against mmap's 25-26 s / 1.8 GiB on 39 GiB of artifacts, and 0.6 s warm against 3 s. No unsafe; memmap2 leaves the check crate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f15fa4f to
d9f0aa3
Compare
OutputTypesValid,MissingRuntimeDepsandCheckCache::lib_symbolsread every artifact file whole (std::fs::read) to parse its ELF header, imports and dynamic symbols. A full-catalog check sweep therefore reads every byte of every output on both arches, though the parsers touch a few pages per file. On a host whose page cache is cold, the sweep becomes disk-bound.This maps the file read-only instead (
map_output→memmap2::Mmap, empty files special-cased since a zero-length map is refused) and advisesMADV_RANDOM, because default readahead pulls a window behind every fault.object::File::parsetakes the same&[u8], so no parsing code changes.Measured
A standalone benchmark doing what these checkers do to each file (open,
object::File::parse, walk imports and dynamic symbols): 261,820 files / 39.2 GiB of realbuilt/artifacts, 20 threads (the check semaphore), page cache evicted per file withposix_fadvise(DONTNEED)for the cold runs. Two runs each:MADV_RANDOM(this PR)Identical results in every run (5,592 ELF files parsed, 545,713 imports).
Why it matters
gominimal/build-servers#210: merge-queue runs sweep the full catalog (16,560 objects). Over the last 7 days they took 83 s median and up to 474 s, and the slow ones are the first run after an on-demand VM boots. In Cloud Trace, the same 1,106 package checks averaged 7.9 s cold vs 2.0 s warm, with over 90% of span time idle: blocking whole-file reads on async workers stalled every other check on those workers.
checkcrate: fmt, clippy-D warnings, 17 tests pass (Linux).🤖 Generated with Claude Code
Summary by CodeRabbit