-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Diff against a dedicated exec version of the baseline configuration #18002
Conversation
@com6056 Could you verify that this, together with bazel-contrib/rules_go#3522, fixes the rebuilds for you? |
830e689
to
ca0a5e9
Compare
By "previous" do you mean |
src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
Show resolved
Hide resolved
Yep looks like both changes fix the issue, thanks so much, really appreciate it! 🎉 |
Now that the host configuration is gone, fixed changes to the `CoreOptions` when switching to the exec configuration are better handled in `CoreOptions#getExec`.
ca0a5e9
to
dafb728
Compare
@sdtwigg Rebased onto master, it's much smaller now. |
When `--experimental_output_directory_naming_scheme` is set to `diff_against_baseline`, the current configuration is diffed against a baseline and a hash of the diff is appended to the output directory. If a target that is built in the exec configuration transitions further, the diff would, before this change, include the (substantial) set of flags that differ between the top-level target and exec configuration. This made caching less efficient across builds that change flags that are reset by the exec configuration, e.g. `--collect_code_coverage`. This is fixed by comparing exec configuration against a dedicated exec version of the top-level target configuration, which is safe since `is exec configuration` itself is explicit in the output path.
596a606
to
6bcc61a
Compare
Heya! Sorry for the response delay here, thought I had responded earlier and was tagged in a way that I didn't get any more notifs to here. The good news is that we actually discovered this issue internally and had been looking into ways to fix it. (There were some other considerations and time spent as to whether other native transitions need a similar treatment. The short story there is that the exec transition is likely 'special' here and the only one of major issue.) The bad news is that I am a bit picky about the implementation here because I have my own internal version of doing this that I was still playing with and did not yet release. (Fun trivia: f7829f8 was done as part of writing those internal versions.) In particular, we want to avoid adding new PrecomputedValue types so it would be preferable to instead get that post-exec baseline as a new SkyFunction call. Similarly, to accommodate future changes to the execution transition, would be best actually get a real ExecutionTransition via ExecutionTransitionFactory.createFactory and use that. Finally, to aid experimentation, was doing this by adding a new mode to To save you time, I am tempted to just submit my internal version instead unless you think one of the differences discussed above was irreconcilable. I do want to leave this open for a little bit longer to discuss what performance benefits users saw (as the main change to output directory calculation is still morally equivalent). In particular, I am especially curious if just these exec transition changes here cleaned up the performance issue without needing other cleanups to native transitions as this data would help me fast-forward some other tests I was doing. |
The approach you describe sounds more conceptual, which I like. I agree that you would be in a better position than me to apply that change, so please just go ahead - I will open a follow-up PR that switches Bazel over to the new mode when it becomes available. For rules_go this change was everything to prevent rebuilds of our exec tools after ensuring that we do not leak coverage-related C++ flags into their builds (see bazel-contrib/rules_go#3522). |
…tory_naming_scheme. To support, a new SkyFunction is introduced, BaselineOptionsFunction, with corresponding SkyValue and SkyKey. Also, a new function ExecutionTransitionFactory.createTransition was added to immediately create an ExecutionTransition. When --experimental_output_directory_naming_scheme=diff_against_dynamic_baseline, the behavior is similar to diff_against_baseline, except when "is Exec" is true (that is, an ExecutionTransition has previously been applied), the baseline is chosen to be result of the ExecutionTransition being applied to the normal baseline. This is meant to resolve an issue with performance when using remote execution engines that collate requests from multiple users. They were seeing varying output paths for actions in an execution configuration depending on how much 'resetting' the exec transition was doing because of varying user commandlines. This relates to #14023 and #18002 PiperOrigin-RevId: 537097551 Change-Id: I72966406b7b8af0174a81b638bf0d1fb62466653
Closing in favor of #18561 |
…tory_naming_scheme. To support, a new SkyFunction is introduced, BaselineOptionsFunction, with corresponding SkyValue and SkyKey. Also, a new function ExecutionTransitionFactory.createTransition was added to immediately create an ExecutionTransition. When --experimental_output_directory_naming_scheme=diff_against_dynamic_baseline, the behavior is similar to diff_against_baseline, except when "is Exec" is true (that is, an ExecutionTransition has previously been applied), the baseline is chosen to be result of the ExecutionTransition being applied to the normal baseline. This is meant to resolve an issue with performance when using remote execution engines that collate requests from multiple users. They were seeing varying output paths for actions in an execution configuration depending on how much 'resetting' the exec transition was doing because of varying user commandlines. This relates to bazelbuild#14023 and bazelbuild#18002 PiperOrigin-RevId: 537097551 Change-Id: I72966406b7b8af0174a81b638bf0d1fb62466653
…tory_naming_scheme. To support, a new SkyFunction is introduced, BaselineOptionsFunction, with corresponding SkyValue and SkyKey. Also, a new function ExecutionTransitionFactory.createTransition was added to immediately create an ExecutionTransition. When --experimental_output_directory_naming_scheme=diff_against_dynamic_baseline, the behavior is similar to diff_against_baseline, except when "is Exec" is true (that is, an ExecutionTransition has previously been applied), the baseline is chosen to be result of the ExecutionTransition being applied to the normal baseline. This is meant to resolve an issue with performance when using remote execution engines that collate requests from multiple users. They were seeing varying output paths for actions in an execution configuration depending on how much 'resetting' the exec transition was doing because of varying user commandlines. This relates to bazelbuild#14023 and bazelbuild#18002 PiperOrigin-RevId: 537097551 Change-Id: I72966406b7b8af0174a81b638bf0d1fb62466653
…tory_naming_scheme. To support, a new SkyFunction is introduced, BaselineOptionsFunction, with corresponding SkyValue and SkyKey. Also, a new function ExecutionTransitionFactory.createTransition was added to immediately create an ExecutionTransition. When --experimental_output_directory_naming_scheme=diff_against_dynamic_baseline, the behavior is similar to diff_against_baseline, except when "is Exec" is true (that is, an ExecutionTransition has previously been applied), the baseline is chosen to be result of the ExecutionTransition being applied to the normal baseline. This is meant to resolve an issue with performance when using remote execution engines that collate requests from multiple users. They were seeing varying output paths for actions in an execution configuration depending on how much 'resetting' the exec transition was doing because of varying user commandlines. This relates to bazelbuild#14023 and bazelbuild#18002 PiperOrigin-RevId: 537097551 Change-Id: I72966406b7b8af0174a81b638bf0d1fb62466653
…tory_naming_scheme. To support, a new SkyFunction is introduced, BaselineOptionsFunction, with corresponding SkyValue and SkyKey. Also, a new function ExecutionTransitionFactory.createTransition was added to immediately create an ExecutionTransition. When --experimental_output_directory_naming_scheme=diff_against_dynamic_baseline, the behavior is similar to diff_against_baseline, except when "is Exec" is true (that is, an ExecutionTransition has previously been applied), the baseline is chosen to be result of the ExecutionTransition being applied to the normal baseline. This is meant to resolve an issue with performance when using remote execution engines that collate requests from multiple users. They were seeing varying output paths for actions in an execution configuration depending on how much 'resetting' the exec transition was doing because of varying user commandlines. This relates to bazelbuild#14023 and bazelbuild#18002 PiperOrigin-RevId: 537097551 Change-Id: I72966406b7b8af0174a81b638bf0d1fb62466653
…ut_direc… (#19514) …tory_naming_scheme. To support, a new SkyFunction is introduced, BaselineOptionsFunction, with corresponding SkyValue and SkyKey. Also, a new function ExecutionTransitionFactory.createTransition was added to immediately create an ExecutionTransition. When --experimental_output_directory_naming_scheme=diff_against_dynamic_baseline, the behavior is similar to diff_against_baseline, except when "is Exec" is true (that is, an ExecutionTransition has previously been applied), the baseline is chosen to be result of the ExecutionTransition being applied to the normal baseline. This is meant to resolve an issue with performance when using remote execution engines that collate requests from multiple users. They were seeing varying output paths for actions in an execution configuration depending on how much 'resetting' the exec transition was doing because of varying user commandlines. This relates to #14023 and #18002 PiperOrigin-RevId: 537097551 Change-Id: I72966406b7b8af0174a81b638bf0d1fb62466653 Co-authored-by: Googler <twigg@google.com>
When
--experimental_output_directory_naming_scheme
is set todiff_against_baseline
, the current configuration is diffed against abaseline and a hash of the diff is appended to the output directory.
If a target that is built in the exec configuration transitions further,
the diff would, before this change, include the (substantial) set of
flags that differ between the top-level target and exec configuration.
This made caching less efficient across builds that change flags that
are reset by the exec configuration, e.g.
--collect_code_coverage
.This is fixed by comparing exec configuration against a dedicated exec
version of the top-level target configuration, which is safe since
is exec configuration
itself is explicit in the output path even withnon-default values of
--experimental_exec_configuration_distinguisher
.This includes a cleanup: Now that the host configuration is gone, fixed
changes to the
CoreOptions
when switching to the exec configuration arebetter handled in
CoreOptions#getExec
.Work towards bazel-contrib/rules_go#3472