Analysis phase improvements for rules #4013
Comments
|
While you are down this path, I would like to note that we were passing a lot of duplicated information through our providers. As a result, a query using So a potential optimization would be to reduce the duplicated information between different providers we are returning for each rule. |
|
Also, it would be nice to know how you benchmark these improvements and what improvement metrics you are seeing on your end. I would love to rerun that using BuildBuddy repo with/without RBE. |
|
The providers also have lots of depsets of structs where structs of depsets would be much more memory-efficient. While technically public API, I would say change whatever shows clear benefits. |
|
Agree. Here is an example of running a simple cquery on our repo that causes Bazel to crash If we run Some stuff like |
|
@fmeum @sluongng I am playing with I did notice the provider are a mess, I will poke at it a bit and see if we can clean things up there. I can repro the #4023, https://github.com/bazelbuild/rules_go/pull/4022/files |
|
@fmeum @sluongng So I did some investigation, and I think it's currently hard to make big improvements to the ArchiveData setup. The problem is The doc comment summarizes it well: I'm guessing breaking this functionality is not a great option (though I am pretty sure this is what we did at Dropbox; we just didn't support external tests in the same package). Failing that, it seems like we may be able to do this in a more Bazel-friendly way with an aspect; we may want to apply it conditionally only when there are external tests. May need some Gazelle support. What do you guys think? |
|
hmm, I wonder if there is a way for us to detect this early on, perhaps during the first GoCompilePkg of the internal test, to just produce 2 sets of outputs: original archive and re-compiled archive. If the re-compilation is not needed, then just create empty files. Then let the external test depend on both of those output groups and determine which one to use at runtime of So we push more work to the action itself and let analysis be lighter weight. |
|
For context, you could see how this logic is used via And comparing the 2 actions, they are fairly similar: @@ -1,12 +1,11 @@
-action 'GoCompilePkg tests/core/go_test/indirect_import_test.internal.a'
+action 'GoCompilePkg tests/core/go_test/indirect_import_test.internal.recompileinternal.a'
Mnemonic: GoCompilePkg
Target: //tests/core/go_test:indirect_import_test
Configuration: darwin_arm64-fastbuild
Execution platform: @internal_platforms_do_not_use//host:host
- ActionKey: 403aabc176ca6f0cb305a3eb00a68da2c7ed1999879ead44ca60b3d98740a562
+ ActionKey: bd10d9739be121241ad208d405c8645e1665eff3da8213e723c4771123046265
Inputs: [
bazel-out/darwin_arm64-fastbuild/bin/stdlib_/pkg,
- bazel-out/darwin_arm64-fastbuild/bin/tests/core/go_test/indirect_import_dep.x,
bazel-out/darwin_arm64-opt-exec-ST-a828a81199fe/bin/external/go_sdk/builder_reset/builder,
bazel-out/darwin_arm64-opt-exec-ST-a828a81199fe/bin/external/go_sdk/packages.txt,
external/go_sdk/bin/gofmt,
@@ -36,7 +35,7 @@
tests/core/go_test/indirect_import_lib.go,
tests/core/go_test/indirect_import_x_test.go,
]
- Outputs: [bazel-out/darwin_arm64-fastbuild/bin/tests/core/go_test/indirect_import_test.internal.a, bazel-out/darwin_arm64-fastbuild/bin/tests/core/go_test/indirect_import_test.internal.x]
+ Outputs: [bazel-out/darwin_arm64-fastbuild/bin/tests/core/go_test/indirect_import_test.internal.recompileinternal.a, bazel-out/darwin_arm64-fastbuild/bin/tests/core/go_test/indirect_import_test.internal.recompileinternal.x]
Environment: [CGO_ENABLED=1, GOARCH=arm64, GOEXPERIMENT=nocoverageredesign, GOOS=darwin, GOPATH=, GOROOT_FINAL=GOROOT, GOTOOLCHAIN=local]
Command Line: (exec bazel-out/darwin_arm64-opt-exec-ST-a828a81199fe/bin/external/go_sdk/builder_reset/builder \
compilepkg \
@@ -58,8 +57,8 @@
bazel-out/darwin_arm64-fastbuild/bin \
-embedlookupdir \
tests/core/go_test \
- -arc \
- 'github.com/bazelbuild/rules_go/tests/core/go_test/indirect_import_dep=github.com/bazelbuild/rules_go/tests/core/go_test/indirect_import_dep=bazel-out/darwin_arm64-fastbuild/bin/tests/core/go_test/indirect_import_dep.x' \
+ -recompile_internal_deps \
+ github.com/bazelbuild/rules_go/tests/core/go_test/indirect_import_dep \
-importpath \
github.com/bazelbuild/rules_go/tests/core/go_test/indirect_import \
-p \
@@ -67,9 +66,9 @@
-package_list \
bazel-out/darwin_arm64-opt-exec-ST-a828a81199fe/bin/external/go_sdk/packages.txt \
-lo \
- bazel-out/darwin_arm64-fastbuild/bin/tests/core/go_test/indirect_import_test.internal.a \
+ bazel-out/darwin_arm64-fastbuild/bin/tests/core/go_test/indirect_import_test.internal.recompileinternal.a \
-o \
- bazel-out/darwin_arm64-fastbuild/bin/tests/core/go_test/indirect_import_test.internal.x \
+ bazel-out/darwin_arm64-fastbuild/bin/tests/core/go_test/indirect_import_test.internal.recompileinternal.x \
-testfilter \
exclude \
-gcflags \(edit: new diff with inputs split into multi-lines and sorted) |
|
So if we ever go down this road:
This could be quite a lot of work though 🤔 |
|
@sluongng how would your proposal cover the following scenario:
I believe what happens today is when compiling foo_test.internal, we create a |
I did not notice that the transitive deps also need to be recompiled. Inspecting the query result a bit closer, we have something like this Bazel Aquerywhich, could be summarized as So yeah, if we want to trip this, it would have to be some aspect-like setup to trigger the recompiliation of transitive deps. |
|
What's the use case for transitive (rather than direct deps only) recompiles? Does Gazelle ever generate target structures in which this is needed? If the answer is no, we could think of deprecating this feature. |
|
I wonder how |
|
So I recreated Then do a build like this and by analyzing the build log, we get So interestingly:
Here is the full log https://gist.github.com/sluongng/e2d1cc724ad441c8664bc784e4f739ac |
|
@sluongng I'm still wading through the log file, there's a ton in there. Do you have a sense of what we need to fix yet? (Or how the normal go tooling is able to avoid the transitive recopmilation issue?) |
|
discussed with @dzbarsky a bit on Slack about this:
|
What version of rules_go are you using?
Tip
What version of gazelle are you using?
n/a
What version of Bazel are you using?
7.3 rc1
Does this issue reproduce with the latest releases of all the above?
yes
What operating system and processor architecture are you using?
darwin arm64
What did you do?
bazel build --nobuild --starlark_cpu_profileWhat did you expect to see?
A relatively clean profile
What did you see instead?
Opportunities to improve things
The text was updated successfully, but these errors were encountered: