Call GetCanonicalPath only if needed to avoid FS overhead

[thelvis4]

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 2c64f60, 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

[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

apple_support/crosstool/cc_toolchain_config.bzl

Lines 1406 to 1436 in 9e5f2d5

	# A private feature that is used to embed absolute source paths in coverage builds.
	#
	# If enabled, coverage builds will use a `-coverage-prefix-map` that remaps
	# the current working directory to a canonical location, which permits
	# coverage representation in non-sandboxed builds with tools that expect
	# absolute paths such as Xcode.
	#
	# This feature should only be used with non-sandboxed builds inside tools such as
	# Xcode, and enabling it effectively breaks Bazel's ability to rely on the
	# remote cache those builds. It should not be enabled by users of the toolchain.
	coverage_prefix_map_absolute_sources_non_hermetic_private_feature = feature(
	name = "_coverage_prefix_map_absolute_sources_non_hermetic",
	flag_sets = [
	flag_set(
	actions = [
	ACTION_NAMES.preprocess_assemble,
	ACTION_NAMES.c_compile,
	ACTION_NAMES.cpp_compile,
	ACTION_NAMES.cpp_module_compile,
	ACTION_NAMES.objc_compile,
	ACTION_NAMES.objcpp_compile,
	],
	flag_groups = [
	flag_group(
	flags = ["-fcoverage-prefix-map=__BAZEL_EXECUTION_ROOT__=__BAZEL_EXECUTION_ROOT_CANONICAL__"],
	),
	],
	),
	],
	requires = [feature_set(features = ["coverage"])],
	)

and only calculate it when that is set

[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

apple_support/crosstool/cc_toolchain_config.bzl

Lines 1406 to 1436 in 9e5f2d5

	# A private feature that is used to embed absolute source paths in coverage builds.
	#
	# If enabled, coverage builds will use a `-coverage-prefix-map` that remaps
	# the current working directory to a canonical location, which permits
	# coverage representation in non-sandboxed builds with tools that expect
	# absolute paths such as Xcode.
	#
	# This feature should only be used with non-sandboxed builds inside tools such as
	# Xcode, and enabling it effectively breaks Bazel's ability to rely on the
	# remote cache those builds. It should not be enabled by users of the toolchain.
	coverage_prefix_map_absolute_sources_non_hermetic_private_feature = feature(
	name = "_coverage_prefix_map_absolute_sources_non_hermetic",
	flag_sets = [
	flag_set(
	actions = [
	ACTION_NAMES.preprocess_assemble,
	ACTION_NAMES.c_compile,
	ACTION_NAMES.cpp_compile,
	ACTION_NAMES.cpp_module_compile,
	ACTION_NAMES.objc_compile,
	ACTION_NAMES.objcpp_compile,
	],
	flag_groups = [
	flag_group(
	flags = ["-fcoverage-prefix-map=__BAZEL_EXECUTION_ROOT__=__BAZEL_EXECUTION_ROOT_CANONICAL__"],
	),
	],
	),
	],
	requires = [feature_set(features = ["coverage"])],
	)

and only calculate it when that is set

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

[adincebic]

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

[thelvis4]

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