perf: Speedup root_relative_path #509
Conversation
dzbarsky
requested review from
armandomontanez,
c-mita,
comius,
hvadehra,
pzembrod and
trybka
as code owners
cc/common/cc_helper_internal.bzl
Outdated
| """ | ||
| if not file.is_source: | ||
| if file.path.startswith(file.root.path + "/"): | ||
| # TODO(zbarsky): is this always the case? |
There was a problem hiding this comment.
I think that this should always be true and we can unconditionally strip the root path with slash. But I wanted to guarantee parity with the former native implementation, which did this.
There was a problem hiding this comment.
I don't have a strong opinion, though I think as-written the PR is guaranteed not to change the behavior because it checks before stripping. Simplifying it to just strip and dropping the lines below would be a pretty minimal speedup compared to what this already achieves
There was a problem hiding this comment.
I can approve this if you can do some double checking so we can confidently remove this TODO.
Edit: Or update the comment to include reasons/sources of uncertainty.
There was a problem hiding this comment.
I also feel like this should always be true but wasn't sure if Google had any internal weirdness that made the previous impl necessary - didn't want to mix a perf improvement with a potential semantic change. But given the import process will make sure google3 is happy anyway, we can give it a shot! Updated
There was a problem hiding this comment.
https://github.com/bazelbuild/bazel/blob/8eaf6a90db1d0e95fbade4dd2b7e3b8c57d03629/src/main/java/com/google/devtools/build/lib/actions/Artifact.java#L308 should be proof that this is always correct - the call would crash if it isn't.
dzbarsky
force-pushed
the
zbarsky/root_relative_path
branch
from
210737e to
0119ae8
Compare
3 participants
Before:
After:
