Call GetCanonicalPath only if needed to avoid FS overhead
After updating to apple_support 2.0.0, we noticed a significant (~40%) regression in compilation times.
For a large application (~45k clang compilations + few thousands swift module compilations), the overall build time increased from 38 min to 54 min.
The regression is rooted in changes in https://github.com/bazelbuild/apple_support/commit/2c64f60587e02d099ab4e050c33026b415f2afd8, where GetCanonicalPath is called for each argument, regardless if the replacement is needed (ie when code coverage is enabled) or not.
The overhead might vary, but it becomes significant and noticeable for compilation actions with lots of arguments.
This PR changes the logic for replacing __BAZEL_EXECUTION_ROOT_CANONICAL__ to avoid the overhead:
-
coverage_prefix_map_absolute_sources_non_hermetic_private_featuresetsCOVERAGE_PREFIX_MAP_USE_ABSOLUTE_CWD_PATH=1env -
GetCanonicalPathis called only if the environment variable is set (ie if the feature is requested) and its value is assigned tocanonical_cwd -
canonical_cwdis passed down as a parameter andProcessArgumentperforms the replacement if the parameter is non-empty
this change makes sense but instead of using a static variable here can you pass it down how cwd is passed down? or is the fact that it would still run once a problem there? if that is still a problem i think we should add an env var here https://github.com/bazelbuild/apple_support/blob/9e5f2d5d398254232424d1f71675477d2b140b44/crosstool/cc_toolchain_config.bzl#L1406-L1436 and only calculate it when that is set
this change makes sense but instead of using a static variable here can you pass it down how
cwdis passed down? or is the fact that it would still run once a problem there? if that is still a problem i think we should add an env var herehttps://github.com/bazelbuild/apple_support/blob/9e5f2d5d398254232424d1f71675477d2b140b44/crosstool/cc_toolchain_config.bzl#L1406-L1436
and only calculate it when that is set
@Keith updated the PR, please check if it's inline with what you had in mind
@thelvis4 @keith what's the state of this?
@thelvis4 @keith what's the state of this?
@adincebic I think it's good to be merged, would appreciate a review. Snap used a patch similar to this for a while, reduced the overhead we initially saw to 0 for normal builds and to unnoticeable overhead when building for code coverage.