perf: avoid unnecessary nested depset() #1435
Conversation
jbedard
requested review from
alexeagle,
gregmagolan and
soldair
as code owners
internal/node/node.bzl
Outdated
|
|
||
| return [ | ||
| DefaultInfo( | ||
| executable = executable, | ||
| runfiles = ctx.runfiles( | ||
| transitive_files = runfiles, | ||
| transitive_files = depset(runfiles_depsets), |
There was a problem hiding this comment.
the fact that you don't use transitive = runfiles_depsets makes me think the variable is misnamed, and runfiles_depsets is actually just runfiles list
There was a problem hiding this comment.
Yeah I guess it is just plain files, previously those plain files were converted into transitive depsets though. I think this change is ok though, but the var should be renamed...
There was a problem hiding this comment.
Updated to just runfiles
jbedard
force-pushed
the
bazel-perf
branch
2 times, most recently
from
7c1c888
to
c685e73
Compare
| sources_sets = [] | ||
| # Also avoid deap transitive depset()s by creating single array of | ||
| # transitive depset()s | ||
| sources_depsets = [] |
There was a problem hiding this comment.
This is just a rename after rebasing. Revert?
|
Updated. I think the buildkite failure is random? |
|
Or maybe it isn't random? I don't see how that could be related though... |
|
@alexeagle the build failure seems to be the same thing as PRs such as #1445 - so I guess it is random and unrelated to this change? |
|
Yes that test is terribly flaky right now, in that it sometimes but rarely passes. We need to jump on that this week. |
As described in the bazel docs unnecessarily nesting
depsets is a performance anti-pattern.I think all these cases are pretty basic changes?