apple_support icon indicating copy to clipboard operation
apple_support copied to clipboard

Call GetCanonicalPath only if needed to avoid FS overhead

Open thelvis4 opened this issue 2 months ago • 2 comments

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:

  1. coverage_prefix_map_absolute_sources_non_hermetic_private_feature sets COVERAGE_PREFIX_MAP_USE_ABSOLUTE_CWD_PATH=1 env
  2. GetCanonicalPath is called only if the environment variable is set (ie if the feature is requested) and its value is assigned to canonical_cwd
  3. canonical_cwd is passed down as a parameter and ProcessArgument performs the replacement if the parameter is non-empty

thelvis4 avatar Dec 19 '25 22:12 thelvis4

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

keith avatar Dec 19 '25 23:12 keith

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

@Keith updated the PR, please check if it's inline with what you had in mind

thelvis4 avatar Dec 22 '25 23:12 thelvis4

@thelvis4 @keith what's the state of this?

adincebic avatar Feb 21 '26 22:02 adincebic

@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.

thelvis4 avatar Feb 22 '26 02:02 thelvis4