Batch input staging syscalls - #2695
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Some of the pull request description still needs filling in:
Edit the description and this check re-runs on its own. The sections exist because they are the parts a reviewer cannot get from the diff: why the change is needed, how you know it works, and what breaks if it is wrong. |
|
|
HackAttack
changed the title
MarcusSorealheis
left a comment
MarcusSorealheis
left a comment
There was a problem hiding this comment.
I don't like to compare and I know this is still a draft, but @HackAttack this is one of the best contributions we've received from outside contributors in a few years. I'm sure all of the users of Nativelink, and there are hundreds, are very appreciative. Our company certainly is grateful.
The batched helpers take one OPEN_FILE_SEMAPHORE permit for the whole batch and run all syscalls sequentially in one spawn_blocking. Correctness argument (metadata ops hold no fd) is sound, but the permit no longer bounds per-syscall concurrency and one blocking thread is pinned for a whole tree. Given the APFS metadata-lock scar already noted in the comments and that this is "only measured on Linux + tmpfs," it's worth a sentence on the changed backpressure profile before assuming it's neutral elsewhere.
@amankrx and @corcillo we probably need to think about expanding the docs for some explanation or callout on this behavior at some point if it is confusing to operators.
Also, the fs_test.rs additions are great for the primitives, but nothing exercises collect_download_links → create_dir_many → materialize_links end-to-end. A directed worker test over a deep tree mixing exec/plain/zero-digest/symlink/metadata files would lock in the "parents before children" and "inline files make their own parents" invariants this refactor leans on.
I could probably find a handful of nits but I'll come back to that in the AM (PT) after my workout.
I am slightly worried about AlreadyExists on MacOS. I'm going to also look into that. @erneestoc Would be great for you to take it for a spin with your workload.
| /// from one file to one input tree. An action holding an | ||
| /// [`ActionInputLease`] is protected from both, because pinning the digest | ||
| /// prevents the eviction that would rename the blob or fire the callback, | ||
| /// and [`prepare_action_inputs`] always takes one; the exposure is the |
There was a problem hiding this comment.
The safety claim here overclaims, and I think it points at the wrong entry point. prepare_action_inputs doesn't "always take one." It calls prepare_action_inputs_with_lease(..., None) unconditionally (L924-931). The only lease-bearing caller is RunningActionImpl, gated on active_input_leases (L1487), i.e. experimental_active_input_leases, which defaults off. So the exposed case isn't the download_to_directory helper (no callers in-tree). It's the default worker config. cc @palfrey if time permits.
There was a problem hiding this comment.
That matters here specifically because batching widens the resolve→link window from per-file to whole-tree:
A file resolved early now waits for the entire walk + dir creation before its hardlink fires (100s of ms on a 15k-file tree). Under eviction pressure that's where a rename (plain files → NotFound despite the pinned inode) or a variant deletion (exec files, _keepalive: None) becomes an action failure rather than a microsecond flake.
There was a problem hiding this comment.
I'm not asking to close the race in this PR. It's excellent! The hard_link_locked_many upgrade is the right fix. But could we (1) reword this to say the default/unleased config is the exposed one, and (2) add a line of operator guidance (enable leases, or keep max_bytes generous) since the blast radius grew?
|
Also, you need to sign the CLA. If you cannot sign it, we have another process. Just hit me up at marcus at the project's name dot com and we can kick that off. |
|
Thank you! It is in draft because my agent eagerly opened it before I was ready. I will take it out of draft today after fixing the Windows build and tightening the commentary. |
|
Ha! Fair enough. I think Other than the two cross-platform issues and the comment, it's good. Amazing agent steering in any event. |
f927abf to
5f0a7b9
Compare
5f0a7b9 to
125f14f
Compare
125f14f to
0110d3f
Compare
Staging an action's input tree dispatched one `spawn_blocking` per file, each taking a permit from the open-file semaphore. On a toolchain-heavy build that can be tens of millions of `stat(2)` calls whose dispatch cost dominates the syscalls themselves, and the worker spends most of its time in staging rather than actual work. The walk now defers every file that needs nothing stamped on it, which is almost all of them, and the tree resolves and links in batched passes: one `stat` batch for executable variants, one `mkdir` batch, and one `hard_link` batch. Files that do carry metadata, zero-digest files, and symlinks are still materialized inline. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
0110d3f to
d50aee4
Compare
4 participants
What and why
I noticed that a local remote build of my project on NativeLink was far slower than a local sandboxed build, and the CPU was <50% utilized when I expected the build to be CPU-bound. Upon investigation I found that the majority of the time was being spent staging the many, many inputs (we use hermetic toolchains).
This was because staging an action's input tree dispatched one
spawn_blockingper file, each taking a permit from the open-file semaphore. On tmpfs those syscalls take a few microseconds; the time is completely dominated by the dispatch around them.The walk now defers every file with no metadata to stamp, which is nearly all of them, and materializes the tree in three batched passes: resolve executable variants,
mkdir,hard_link. Metadata-bearing files, zero-digest files, and symlinks stay on their existing inline paths and create their own parents.How was this verified?
A 15,000-action Bazel build against a local worker, sampling
/procthrough the run:Worker-side input staging went from 4,598ms to 37ms per action.
Risk
Batching defers a hardlink from resolution time to the end of the tree walk, widening a pre-existing window in which eviction can rename a blob out from under it. Documented on
PendingLink::_keepalivewith its upgrade path; an action holding anActionInputLease(#2675) is not exposed to it.Only measured on Linux + tmpfs.
This change is