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: low cache utilization between builds and tests #71857

Closed
irfansharif opened this issue Oct 22, 2021 · 7 comments · Fixed by #72838
Closed

bazel: low cache utilization between builds and tests #71857

irfansharif opened this issue Oct 22, 2021 · 7 comments · Fixed by #72838
Labels
A-build-system C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-dev-inf

Comments

@irfansharif
Copy link
Contributor

irfansharif commented Oct 22, 2021

Describe the problem

I'm observing that the build artifacts from a {dev,bazel} build are not being used by {dev,bazel} test and vice-versa. When going from building the full cockroach-short binary to running a specific test, the build cache is trampled upon by each command with the results no longer usable by the other or by itself afterwards. This is surprising, is it because of the differing .bazelrc configs between the two? Something else?

To Reproduce

Some numbers from my machine, I'm seeing each step start the build from scratch(-ish) without achieving much cache utilization.

$ dev test pkg/spanconfig/spanconfigstore # 116.655s
$ dev build cockroach-short               # 210.004s
$ dev test pkg/spanconfig/spanconfigstore # 97.852s
$ dev build cockroach-short               # 176.392s

Here are two profiles (open using chrome://tracing), they show that we're pretty much starting from scratch each time.

  • dev.gz (for an unexpected long build step)
  • test.gz (ditto, for test)

Expected behavior

Build artifacts from a dev build cockroach-short should be re-usable for tests and vice-versa.

Epic CRDB-8036

@irfansharif irfansharif added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-build-system labels Oct 22, 2021
@rickystewart
Copy link
Collaborator

Unfortunately the build and tests are compiled with disjoint gotags (crdb_test for tests, crdb_test_off for non-tests) which erases much of Bazel's ability to cache between the two. I'd be very interested in any changes that allow us to get rid of crdb_test{,_off} to solve this.

@rickystewart
Copy link
Collaborator

We could add some select()s in the BUILD files to enable conditional compilation, and Bazel would have an easier time caching that stuff, but without getting rid of the gotags you still wouldn't see any improvement.

@stevendanna
Copy link
Collaborator

stevendanna commented Oct 22, 2021

I've been playing with this locally recently and I think explicitly setting --disk_cache in .bazelrc.user helps with with the runtime substantially for me at least. You still don't get sharing between test and non-test artifacts, but you do get to keep around build artifacts from more than just the previous run. One issue is that the disk cache grows without bound, so it is occasionally useful to delete it and do a full rebuild.

@jordanlewis
Copy link
Member

crdb_test is an important piece of the way that we test CockroachDB - it's how we enable metamorphic and other expensive checks in our tests, without affecting the performance of our production builds. I don't think it's likely that we'd prioritize removing it anytime soon.

I guess I would expect that Bazel would keep separate caches for each configuration of the gotags, is that not how things work?

@irfansharif
Copy link
Contributor Author

A slack thread about the bazel in-use space here.

@irfansharif
Copy link
Contributor Author

Using an explicit --disk_cache seems to have helped in that build artifacts for dev build and dev test are both kept around for re-use. Something that isn't shared however is the analysis cache -- it seems that bazel keeps only one analysis in the cache and readily discards it whenever the relevant options change (for us it's the buildtags). Filed an issue upstream: bazelbuild/bazel#14179.

@irfansharif
Copy link
Contributor Author

crdb_test is an important piece of the way that we test CockroachDB

Instead of using a build tag (which necessarily generates non-shareable artifacts), I wonder if we can get away with using an init time envvar check instead. Downside: we'd have to compile test-only code for regular builds (will it be much slower?). Upside: we'd be able to re-use artifacts across dev build and dev test.

rickystewart added a commit to rickystewart/cockroach that referenced this issue Nov 16, 2021
In a couple different places in-tree we use the build tags `crdb_test`
and `crdb_test_off` to support conditional compilation. Unfortunately,
this results in low cache utilization when swapping between builds and
tests, since recompiling with different `gotags` causes Bazel to
recompile EVERYTHING, including the entire standard library, with these
different tags. This means that you can't build `cockroach` and then run
a quick test using cached build artifacts, for example.

Fix this by adding a new configuration, `//build/toolchains:crdb_test`.
We use `x_defs` in `pkg/util/BUILD.bazel` to pass in a different string
to the linker in test scenarios and set `CrdbTestBuild` accordingly.
For the remaining few files that directly use `crdb_test` as a build
constraint in `pkg/sql/opt`, we just directly port the conditional
compilation logic to Bazel using `select`.

When the changeover to Bazel is finalized, we can omit the build
constraints entirely in the source files.

Also add logic to `build/bazelutil/check.sh` to detect new uses of these
build constraints and warn people that the logic also needs to be
Bazelfied.

Closes cockroachdb#71857.

Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Nov 16, 2021
In a couple different places in-tree we use the build tags `crdb_test`
and `crdb_test_off` to support conditional compilation. Unfortunately,
this results in low cache utilization when swapping between builds and
tests, since recompiling with different `gotags` causes Bazel to
recompile EVERYTHING, including the entire standard library, with these
different tags. This means that you can't build `cockroach` and then run
a quick test using cached build artifacts, for example.

Fix this by adding a new configuration, `//build/toolchains:crdb_test`.
We use `x_defs` in `pkg/util/BUILD.bazel` to pass in a different string
to the linker in test scenarios and set `CrdbTestBuild` accordingly.
For the remaining few files that directly use `crdb_test` as a build
constraint in `pkg/sql/opt`, we just directly port the conditional
compilation logic to Bazel using `select`.

When the changeover to Bazel is finalized, we can omit the build
constraints entirely in the source files.

Also add logic to `build/bazelutil/check.sh` to detect new uses of these
build constraints and warn people that the logic also needs to be
Bazelfied.

Closes cockroachdb#71857.

Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Nov 17, 2021
In a couple different places in-tree we use the build tags `crdb_test`
and `crdb_test_off` to support conditional compilation. Unfortunately,
this results in low cache utilization when swapping between builds and
tests, since recompiling with different `gotags` causes Bazel to
recompile EVERYTHING, including the entire standard library, with these
different tags. This means that you can't build `cockroach` and then run
a quick test using cached build artifacts, for example.

Fix this by adding a new configuration, `//build/toolchains:crdb_test`.
We use `select()` in `pkg/util/buildutil/BUILD.bazel` to choose between
`crdb_test_on.go` and `crdb_test_off.go` accordingly.

Also add logic to `build/bazelutil/check.sh` to detect new uses of these
build constraints and warn people that the logic also needs to be
Bazelfied.

Closes cockroachdb#71857.

Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Nov 17, 2021
In a couple different places in-tree we use the build tags `crdb_test`
and `crdb_test_off` to support conditional compilation. Unfortunately,
this results in low cache utilization when swapping between builds and
tests, since recompiling with different `gotags` causes Bazel to
recompile EVERYTHING, including the entire standard library, with these
different tags. This means that you can't build `cockroach` and then run
a quick test using cached build artifacts, for example.

Fix this by adding a new configuration, `//build/toolchains:crdb_test`.
We use `select()` in `pkg/util/buildutil/BUILD.bazel` to choose between
`crdb_test_on.go` and `crdb_test_off.go` accordingly.

Also add logic to `build/bazelutil/check.sh` to detect new uses of these
build constraints and warn people that the logic also needs to be
Bazelfied.

Closes cockroachdb#71857.

Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Nov 17, 2021
In a couple different places in-tree we use the build tags `crdb_test`
and `crdb_test_off` to support conditional compilation. Unfortunately,
this results in low cache utilization when swapping between builds and
tests, since recompiling with different `gotags` causes Bazel to
recompile EVERYTHING, including the entire standard library, with these
different tags. This means that you can't build `cockroach` and then run
a quick test using cached build artifacts, for example.

Fix this by adding a new configuration, `//build/toolchains:crdb_test`.
We use `select()` in `pkg/util/buildutil/BUILD.bazel` to choose between
`crdb_test_on.go` and `crdb_test_off.go` accordingly.

Also add logic to `build/bazelutil/check.sh` to detect new uses of these
build constraints and warn people that the logic also needs to be
Bazelfied.

Closes cockroachdb#71857.

Release note: None
rickystewart added a commit to rickystewart/cockroach that referenced this issue Nov 17, 2021
In a couple different places in-tree we use the build tags `crdb_test`
and `crdb_test_off` to support conditional compilation. Unfortunately,
this results in low cache utilization when swapping between builds and
tests, since recompiling with different `gotags` causes Bazel to
recompile EVERYTHING, including the entire standard library, with these
different tags. This means that you can't build `cockroach` and then run
a quick test using cached build artifacts, for example.

Fix this by adding a new configuration, `//build/toolchains:crdb_test`.
We use `select()` in `pkg/util/buildutil/BUILD.bazel` to choose between
`crdb_test_on.go` and `crdb_test_off.go` accordingly.

Also add logic to `build/bazelutil/check.sh` to detect new uses of these
build constraints and warn people that the logic also needs to be
Bazelfied.

Closes cockroachdb#71857.

Release note: None
craig bot pushed a commit that referenced this issue Nov 18, 2021
72838: bazel: don't use different `gotags` for test and non-test scenarios r=RaduBerinde a=rickystewart


In a couple different places in-tree we use the build tags `crdb_test`
and `crdb_test_off` to support conditional compilation. Unfortunately,
this results in low cache utilization when swapping between builds and
tests, since recompiling with different `gotags` causes Bazel to
recompile EVERYTHING, including the entire standard library, with these
different tags. This means that you can't build `cockroach` and then run
a quick test using cached build artifacts, for example.

Fix this by adding a new configuration, `//build/toolchains:crdb_test`.
We use `select()` in `pkg/util/buildutil/BUILD.bazel` to choose between
`crdb_test_on.go` and `crdb_test_off.go` accordingly.

Also add logic to `build/bazelutil/check.sh` to detect new uses of these
build constraints and warn people that the logic also needs to be
Bazelfied.

Closes #71857.

Release note: None

72882: ui: save sort on cache for Transaction page r=maryliag a=maryliag

Previously, a sort selection was not maintained when
the page change (e.g. coming back from Transaction details).
This commits saves the selected value to be used.

Partially adresses #68199

Release note: None

72915: build/README: add more info about how to authenticate for mirroring r=rail a=rickystewart

Release note: None

Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
@craig craig bot closed this as completed in f606f15 Nov 18, 2021
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.

4 participants