Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Apr 6, 2023

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 even with
non-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 are
better handled in CoreOptions#getExec.

Work towards bazel-contrib/rules_go#3472

@fmeum fmeum marked this pull request as ready for review April 6, 2023 06:46
@fmeum fmeum requested a review from a team as a code owner April 6, 2023 06:46
@fmeum fmeum requested review from aiuto and removed request for a team April 6, 2023 06:46
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions labels Apr 6, 2023
@fmeum fmeum requested a review from sdtwigg April 6, 2023 06:46
@fmeum
Copy link
Collaborator Author

fmeum commented Apr 6, 2023

@sdtwigg This is a small change that would help prevent certain cases in which diff_against_baseline could potentially be worse than the previous output directory suffix scheme across builds.

It is stacked on #16910, so please ignore the first commit.

@fmeum
Copy link
Collaborator Author

fmeum commented Apr 6, 2023

@com6056 Could you verify that this, together with bazel-contrib/rules_go#3522, fixes the rebuilds for you?

@fmeum fmeum force-pushed the add-exec-baseline branch 2 times, most recently from 830e689 to ca0a5e9 Compare April 6, 2023 09:24
@gregestren
Copy link
Contributor

@sdtwigg This is a small change that would help prevent certain cases in which diff_against_baseline could potentially be worse than the previous output directory suffix scheme across builds.

By "previous" do you mean --experimental_output_directory_naming_scheme=legacy and --experimental_exec_configuration_distinguisher=legacy? (trying to remember which combination of flags we expect when!)

@com6056
Copy link

com6056 commented Apr 6, 2023

@com6056 Could you verify that this, together with bazelbuild/rules_go#3522, fixes the rebuilds for you?

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`.
@fmeum
Copy link
Collaborator Author

fmeum commented Apr 13, 2023

@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.
@fmeum fmeum force-pushed the add-exec-baseline branch 3 times, most recently from 596a606 to 6bcc61a Compare April 14, 2023 09:28
@sdtwigg
Copy link
Contributor

sdtwigg commented Apr 24, 2023

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 --experimental_output_directory_naming_scheme called diff_against_dynamic_baseline.

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.

@fmeum
Copy link
Collaborator Author

fmeum commented Apr 25, 2023

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

@gregestren gregestren removed the awaiting-review PR is awaiting review from an assigned reviewer label May 10, 2023
copybara-service bot pushed a commit that referenced this pull request Jun 1, 2023
…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
@fmeum
Copy link
Collaborator Author

fmeum commented Jun 6, 2023

Closing in favor of #18561

@fmeum fmeum closed this Jun 6, 2023
@fmeum fmeum deleted the add-exec-baseline branch June 6, 2023 20:04
fmeum pushed a commit to fmeum/bazel that referenced this pull request Sep 13, 2023
…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
fmeum pushed a commit to fmeum/bazel that referenced this pull request Sep 14, 2023
…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
fmeum pushed a commit to fmeum/bazel that referenced this pull request Sep 14, 2023
…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
fmeum pushed a commit to fmeum/bazel that referenced this pull request Sep 14, 2023
…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
iancha1992 pushed a commit that referenced this pull request Sep 14, 2023
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants