Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: improve execution requirements of all copy files/directory rules for better perf #79

Merged

Conversation

gregmagolan
Copy link
Member

@gregmagolan gregmagolan commented Apr 14, 2022

Inspired by bazelbuild/rules_nodejs#3410

Copying file & directories is entirely IO-bound and there is no point doing this work
remotely.

Also, remote-execution does not allow source directory inputs, see
bazelbuild/bazel@c64421b So we must
not attempt to execute remotely in that case.

There is also no point pulling the output file or directory from the remote cache since the
bytes to copy are already available locally. Conversely, no point in writing to the cache if
no one has any reason to check it for this action.

Read and writing to disk cache is disabled as well primarily to reduce disk usage on the local
machine. A disk cache hit of a directory copy could be slghtly faster than a copy since the
disk cache stores the directory artifact as a single entry, but the slight performance bump
comes at the cost of heavy disk cache usage, which is an unmanaged directory that grow beyond
the bounds of the physical disk.

Sandboxing for this action is wasteful as well since there is a 1:1 mapping of input
file/directory to output file/directory and no room for non-hermetic inputs to sneak in to the
input.

I will run benchmarks to measure the impact on copy_directory with no-cache in a follow-up to this 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.

None yet

2 participants