Skip to content

fix(builtin): improve execution requirements for copy file operations - #3413

Merged
alexeagle merged 8 commits into
bazel-contrib:stablefrom
mistic:improve-execution-requirements-copy-file
Apr 18, 2022
Merged

fix(builtin): improve execution requirements for copy file operations#3413
alexeagle merged 8 commits into
bazel-contrib:stablefrom
mistic:improve-execution-requirements-copy-file

Conversation

@mistic

@mistic mistic commented Apr 15, 2022

Copy link
Copy Markdown
Contributor

This PR is recreating what was done at bazel-contrib/bazel-lib#79. It doesn't solve the performance decreasing introduced on v5 for macOS but it help by a small percentage and something we should have anyway. It can also be backported into 4.x I think.

@mistic
mistic marked this pull request as ready for review April 15, 2022 17:09
@mistic

mistic commented Apr 15, 2022

Copy link
Copy Markdown
Contributor Author

The failure is one that already happened a couple times on other previous builds and seems something flaky not related with that PR.

@gregmagolan

Copy link
Copy Markdown
Collaborator

Retrying that windows step

@gregmagolan

Copy link
Copy Markdown
Collaborator

Thanks for the PR @mistic. No sandbox should export_directories_only bit faster. I'd like to run some benchmarks on not using the disk cache with a large set on npm deps. How large is your set of npm deps that you're seeing performance issues with the TreeArtifact on?

@mistic

mistic commented Apr 18, 2022

Copy link
Copy Markdown
Contributor Author

No sandbox should export_directories_only bit faster

That is definitely the case on 4.x but doesn't seem to be anymore in 5.x since the implementation of the feature was changed 😬

@gregmagolan I have around ~833 top level node_modules overall listed in the package.json (dev + prod) (which expands into more modules as they have dependencies too) https://github.com/elastic/kibana/blob/main/package.json#L100 but the average package build should have around ~15 top level modules dependencies (plus their transitive deps) https://github.com/elastic/kibana/blob/main/packages/kbn-config/BUILD.bazel#L49

From 4.x to 5.x for the same codebase and the same bazel rule set the build processes with exports_directories_only jumped from 945 into ~3400 which I think is part of the performance problem I'm seeing as the performance decreased by around 3.5x.

@alexeagle

Copy link
Copy Markdown
Collaborator

@mistic this tiny clean-up PR is going to merge, so that last comment for Greg will be orphaned. I don't think this is the best thread to go into those details since it's orthogonal to this change.

@alexeagle alexeagle left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close, thanks!

Comment thread third_party/github.com/bazelbuild/bazel-skylib/rules/copy_common.bzl Outdated
@alexeagle
alexeagle merged commit 43e478d into bazel-contrib:stable Apr 18, 2022
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.

3 participants