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

bazel: investigate performance impact of nogo #73944

Closed
Tracked by #75453
rickystewart opened this issue Dec 16, 2021 · 3 comments · Fixed by #79057
Closed
Tracked by #75453

bazel: investigate performance impact of nogo #73944

rickystewart opened this issue Dec 16, 2021 · 3 comments · Fixed by #79057
Assignees
Labels
A-build-system C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-dev-inf

Comments

@rickystewart
Copy link
Collaborator

rickystewart commented Dec 16, 2021

Compare cold and incremental build times between the following configurations:

  1. Standard cockroach-short build
  2. cockroach-short build with --config nonogo
  3. Control: delete the nogo = ... argument from the go_register_toolchains() call in WORKSPACE.

For cold build times, we expect nogo will add a substantial initial startup cost before builds begin (this is case (1) above). This is because it needs to compile all the Analysis passes before building the actual Go binary, which does take a long time. (Unfortunately, because one of our passes depends on cockroachdb/errors which has proto dependencies, we need to compile all the C++ code as part of nogo.) However, you shouldn't have to repay that cost until the nogo config is updated. (2) and (3) should have similar overall build times for cold builds.

For incremental builds, (1), (2), and (3) should all be roughly similar. If (1) takes much longer than (2) or (3), or (2) takes much longer than (3), that is a problem.

Epic CRDB-8036

Jira issue: CRDB-11822

@rickystewart rickystewart added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-build-system T-dev-inf labels Dec 16, 2021
@irfansharif
Copy link
Contributor

Another thing worth measuring: performance impact of actually running linters as part of dev build {short}, even if built with --config nogo.

@irfansharif
Copy link
Contributor

irfansharif commented Mar 7, 2022

Haven't investigated this fully/scientifically, but nogo has non-neglible overhead. I tried the following experiment on my GCE worker:

  • dev build short (prime cache with nogo artifacts)
  • dev build short -- --config=nonogo (prime cache with nonogo artifacts)
  • Edit a random pkg/roachpb file to invalidate base package dependency
  • Time dev build short vs. dev build short --config=nonogo for the incremental build
2:31:05s (nogo) vs. 1:51:02s (nonogo)

Tried this a few times when comparing make incremental builds vs. bazel when editing pkg/roachpb, was able to reliably see that nogo has overhead.

1:24.47s (make) vs. 2:31.40s (bazel,nogo)
1:24.95s (make) vs. 2:02.96s (bazel,nogo)
1:25:36s (make) vs. 1:42:62s (bazel, nonogo)
1:28:21s (make) vs. 1:45:65s (bazel, nonogo)
1:24:51s (make) vs. 1:46:09s (bazel, nonogo)

@irfansharif
Copy link
Contributor

Earlier iteration of "should we default to building with nogo": #74540.

rickystewart added a commit to rickystewart/cockroach that referenced this issue Mar 30, 2022
Up until this point `nogo` has been enabled by default and we've
required opting out with `--config nonogo`. Unfortunately `nogo` has a
non-negligible performance impact (see cockroachdb#73944) so having it on by
default is not what we want to be doing. Preserving the current behavior
as opt-in is still desirable though.

We have `dev doctor` advise about this case and recommend that you
explicitly set one of `--config lintonbuild` or
`--config nolintonbuild`. If you have neither or both configured,
`doctor` will detect that. (You can still override one with the other on
the command line.) We also have `dev lint` build `cockroach-short` with
`nogo` if running in non-`short` mode to cover that case for people who
have opted out of `nogo`.

`--config nonogo` is still supported as an alias for `nolintonbuild`.

Closes cockroachdb#73944.
Closes cockroachdb#78666.

Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Mar 30, 2022
Up until this point `nogo` has been enabled by default and we've
required opting out with `--config nonogo`. Unfortunately `nogo` has a
non-negligible performance impact (see cockroachdb#73944) so having it on by
default is not what we want to be doing. Preserving the current behavior
as opt-in is still desirable though.

We have `dev doctor` advise about this case and recommend that you
explicitly set one of `--config lintonbuild` or
`--config nolintonbuild`. If you have neither or both configured,
`doctor` will detect that. (You can still override one with the other on
the command line.) We also have `dev lint` build `cockroach-short` with
`nogo` if running in non-`short` mode to cover that case for people who
have opted out of `nogo`.

`--config nonogo` is still supported as an alias for `nolintonbuild`.

Closes cockroachdb#73944.
Closes cockroachdb#78666.

Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Mar 30, 2022
Up until this point `nogo` has been enabled by default and we've
required opting out with `--config nonogo`. Unfortunately `nogo` has a
non-negligible performance impact (see cockroachdb#73944) so having it on by
default is not what we want to be doing. Preserving the current behavior
as opt-in is still desirable though.

We have `dev doctor` advise about this case and recommend that you
explicitly set one of `--config lintonbuild` or
`--config nolintonbuild`. If you have neither or both configured,
`doctor` will detect that. (You can still override one with the other on
the command line.) We also have `dev lint` build `cockroach-short` with
`nogo` if running in non-`short` mode to cover that case for people who
have opted out of `nogo`.

`--config nonogo` is still supported as an alias for `nolintonbuild`.

Closes cockroachdb#73944.
Closes cockroachdb#78666.

Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Mar 31, 2022
Up until this point `nogo` has been enabled by default and we've
required opting out with `--config nonogo`. Unfortunately `nogo` has a
non-negligible performance impact (see cockroachdb#73944) so having it on by
default is not what we want to be doing. Preserving the current behavior
as opt-in is still desirable though.

We have `dev doctor` advise about this case and recommend that you
explicitly set one of `--config lintonbuild` or
`--config nolintonbuild`. If you have neither or both configured,
`doctor` will detect that. (You can still override one with the other on
the command line.) We also have `dev lint` build `cockroach-short` with
`nogo` if running in non-`short` mode to cover that case for people who
have opted out of `nogo`.

`--config nonogo` is still supported as an alias for `nolintonbuild`.

Closes cockroachdb#73944.
Closes cockroachdb#78666.

Release note: None
craig bot pushed a commit that referenced this issue Mar 31, 2022
78527: roachtest: de-flake sstable corruption test r=tbg a=nicktrav

roachtest: de-flake sstable corruption test

Currently, the `sstable-corruption/table` randomly selects a number of
SSTables on each node to corrupt, before restarting the cluster and
running a workload in an attempt to notice the corruption. The test uses
a monitor to monitor for node death, and also run the background
workload.

There exists a race where monitor's workload goroutine can exit soon
after starting (e.g. due to #77627), and _before_ the monitor encounters
the failure of a node. The test expects an error to be returned by the
monitor, which fails due to the former error being nil.

Address the race by continuing to run the workload even in the case of
failure, exiting only when a timeout is exceeded (due to not
encountering corruption within the allotted time), or other context
cancellation (i.e. a node death _was_ observed by the monitor and the
test is being torn down).

Release note: None.

79025: catalog: protobuf and cluster versioning for table statistics forecasts r=rytaft a=michae2

Add a field to the TableDescriptor for per-table statistics forecasting.
We'll want this when backporting statistics forecasts to 22.1.

Release note: None

79057: dev,bazel: don't do nogo checks by default; have doctor advise r=mari-crl a=rickystewart

Up until this point `nogo` has been enabled by default and we've
required opting out with `--config nonogo`. Unfortunately `nogo` has a
non-negligible performance impact (see #73944) so having it on by
default is not what we want to be doing. Preserving the current behavior
as opt-in is still desirable though.

We have `dev doctor` advise about this case and recommend that you
explicitly set one of `--config lintonbuild` or
`--config nolintonbuild`. If you have neither or both configured,
`doctor` will detect that. (You can still override one with the other on
the command line.) We also have `dev lint` build `cockroach-short` with
`nogo` if running in non-`short` mode to cover that case for people who
have opted out of `nogo`.

`--config nonogo` is still supported as an alias for `nolintonbuild`.

Closes #73944.
Closes #78666.

Release note: None

Co-authored-by: Nick Travers <[email protected]>
Co-authored-by: Michael Erickson <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
@craig craig bot closed this as completed in 7b5f118 Mar 31, 2022
rickystewart added a commit to rickystewart/cockroach that referenced this issue Apr 1, 2022
Up until this point `nogo` has been enabled by default and we've
required opting out with `--config nonogo`. Unfortunately `nogo` has a
non-negligible performance impact (see cockroachdb#73944) so having it on by
default is not what we want to be doing. Preserving the current behavior
as opt-in is still desirable though.

We have `dev doctor` advise about this case and recommend that you
explicitly set one of `--config lintonbuild` or
`--config nolintonbuild`. If you have neither or both configured,
`doctor` will detect that. (You can still override one with the other on
the command line.) We also have `dev lint` build `cockroach-short` with
`nogo` if running in non-`short` mode to cover that case for people who
have opted out of `nogo`.

`--config nonogo` is still supported as an alias for `nolintonbuild`.

Closes cockroachdb#73944.
Closes cockroachdb#78666.

Release note: None
fqazi pushed a commit to fqazi/cockroach that referenced this issue Apr 4, 2022
Up until this point `nogo` has been enabled by default and we've
required opting out with `--config nonogo`. Unfortunately `nogo` has a
non-negligible performance impact (see cockroachdb#73944) so having it on by
default is not what we want to be doing. Preserving the current behavior
as opt-in is still desirable though.

We have `dev doctor` advise about this case and recommend that you
explicitly set one of `--config lintonbuild` or
`--config nolintonbuild`. If you have neither or both configured,
`doctor` will detect that. (You can still override one with the other on
the command line.) We also have `dev lint` build `cockroach-short` with
`nogo` if running in non-`short` mode to cover that case for people who
have opted out of `nogo`.

`--config nonogo` is still supported as an alias for `nolintonbuild`.

Closes cockroachdb#73944.
Closes cockroachdb#78666.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-system C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-dev-inf
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants