Optimize args and inputs construction #1610
Conversation
go/private/actions/compile.bzl
Outdated
| inputs = (sources + [go.package_list] + | ||
| [archive.data.file for archive in archives] + | ||
| go.sdk_tools + go.stdlib.files) | ||
| inputs = sets.union(sources, [go.package_list], |
There was a problem hiding this comment.
Don't use a depset here.
In general, depsets should be only used for accumulating transitive information. Basically, things that are accumulated across a subgraph of rules then expanded once at the top (for linking) should be depsets. Nothing else should be.
Constructing an individual depset is relatively expensive, since they do some de-duplication in the set itself. For compiling, we don't need the deduplication, so it's better to build the input list by concatenation.
There was a problem hiding this comment.
It is totally possible that my understanding is incorrect, but I believe that there are these benefits of using a depset in such a situation:
- Memory savings - list is not concatenated when the rule is executed and hence the memory is not allocated to hold it. Instead, depset is just holding references to parts (lists/dpsets).
- Execution time savings - I assume that constructing a depset is cheap, faster than list concatenation.
Eventually, of course, depset needs to be expanded into a (deduplicated) list and that's when the price is paid - both for deduplication and for memory allocation. However, that price is only paid if that action needs to be executed and never otherwise. And most of the time many actions do not need to run - caching or the output is not required. That is why I believe it is much better to use depsets - memory/cpu cost of analysis (or loading? not sure) phase is reduced and cost is postponed to the execution phase, but only for those actions where it needs to be paid (i.e. where the output is needed).
I may be operating under a wrong assumption here, but I don't see how constructing a depset can be expensive - it would not make any sense, I think. Performance section does not say it directly, but I read it as if it says that to_list() is expensive, not construction of a depset.
There was a problem hiding this comment.
Sadly, implementation of depset is inefficient. It's good to use if you know the asymptotic complexity is strictly better, but compared with something that with the same complexity (list concatenation in thise case), it's almost always worse by a constant factor.
The underlying Java class is NestedSet. That one is used directly within Bazel. SkylarkNestedSet is a layer on top of that.
When you create a NestedSet, the constructor will de-duplicate the direct elements and the transitive sets using a hash table, so construction is O(len(direct) + len(transitive)) in time and space. Several copies are made, and all the direct elements are hashed at least once, so it's a pretty high constant factor, too.
The crucial thing is that the len(transitive) above is the number of transitive sets passed to the constructor, not the number of elements in those sets. So when you're building a big subgraph of values for linking, it's much more efficient than list concatenation.
There was a problem hiding this comment.
Thanks for going into details and explaining! I appreciate that very much. I'll undo the change here and in other places.
go/private/actions/compile.bzl
Outdated
| go.actions.run_shell( | ||
| inputs = sources + go.sdk_files + go.sdk_tools, | ||
| inputs = sets.union(sources, go.sdk_files, go.sdk_tools), |
There was a problem hiding this comment.
Same comment about depsets.
| mnemonic = "GoCompile", | ||
| command = "export GOROOT=$(pwd)/{} && export GOROOT_FINAL=GOROOT && {} {}".format(go.root, go.go.path, " ".join(args)), | ||
| command = "export GOROOT=$(pwd)/{} && export GOROOT_FINAL=GOROOT && {} \"$@\"".format(go.root, " ".join(cmd)), |
There was a problem hiding this comment.
That's cool, I didn't know $@ could be used here.
There was a problem hiding this comment.
This also passes arguments properly - no chance that an unescaped character breaks the script.
go/private/actions/cover.bzl
Outdated
| "--", | ||
| "-mode=set", | ||
| ]) | ||
| go.actions.run( | ||
| inputs = [src] + go.sdk_tools, | ||
| inputs = sets.union([src], go.sdk_tools), |
There was a problem hiding this comment.
Same comment about depsets.
| def _format_archive(d): | ||
| return "{}={}={}".format(d.label, d.importmap, d.file.path) | ||
|
|
||
| def _map_archive(x): |
There was a problem hiding this comment.
I'd rather keep this the way it was. test_archives will pretty much always be 0 or 1 elements, so the filtering using any before should actually be very fast (probably faster than building a dict and doing hash table lookups). I think it was shorter and more readable before, so I prefer that.
I like having _format_archive as a separate function called with map_each though.
There was a problem hiding this comment.
Ok, makes sense - I'll change it back to any(...).
The main motivation to extract this code into a function it to postpone the cost (depset expansion and execution of this code) to the execution phase and, likely, not pay it at all if the output is not needed. See https://docs.bazel.build/versions/master/skylark/performance.html#use-ctx-actions-args-for-command-lines
Basically Args is a way to postpone and avoid the cost of constructing the arguments list. The more processing is delegated to it the better. Currently each instance of the rule where this function is used pays the cost. After this refactoring only the ones where the argument list is actually needed will pay the cost. Does it make sense?
go/private/actions/link.bzl
Outdated
| if d.importmap not in test_importmaps | ||
| ] | ||
|
|
||
| def _map_test_archive(d): |
There was a problem hiding this comment.
Any reason to have this as a separate function?
There was a problem hiding this comment.
No, I just thought it is more readable that way. Happy to just use _format_archive directly.
There was a problem hiding this comment.
I have a mild preference for using _format_archive directly.
I was planning to migrate these arguments into files that could be passed to -importcfg directly. Given what you've said about Args though, maybe it makes sense keep this and do the work in the builder, since it might not need to be done.
go/private/actions/link.bzl
Outdated
| builder_args.add_all(dep_args, before_each = "-dep") | ||
| builder_args.add_all([struct(archive = archive, test_archives = test_archives)], before_each = "-dep", | ||
| map_each = _map_archive) | ||
| builder_args.add_all(test_archives, before_each = "-dep", map_each = _map_test_archive) |
There was a problem hiding this comment.
Instead of doing the work inline here, we postpone it. It will only be done if the action needs to be run.
|
I've rebased on master and got the following warning when running INFO: From GoLink tests/core/cgo/objc/darwin_amd64_stripped/objc_test:
ld: warning: URGENT: building for OSX, but linking in object file (/var/folders/fh/7zp6kdcj3rgdd_7_nmw439_st0zyrt/T/go-link-075800598/000003.o) built for iOS. Note: This will be an error in the future.
ld: warning: object file (/var/folders/fh/7zp6kdcj3rgdd_7_nmw439_st0zyrt/T/go-link-075800598/000003.o) was built for newer iOS version (11.4) than being linked (10.13)
ld: warning: URGENT: building for OSX, but linking in object file (/var/folders/fh/7zp6kdcj3rgdd_7_nmw439_st0zyrt/T/go-link-075800598/000004.o) built for iOS. Note: This will be an error in the future.
ld: warning: object file (/var/folders/fh/7zp6kdcj3rgdd_7_nmw439_st0zyrt/T/go-link-075800598/000004.o) was built for newer iOS version (11.4) than being linked (10.13)
ld: warning: ObjC object file (/var/folders/fh/7zp6kdcj3rgdd_7_nmw439_st0zyrt/T/go-link-075800598/000003.o) was compiled for iOS Simulator, but linking for MacOSXI'm on latest macOS, using bazel 0.15.2. Is it my branch or there is a bug in master or something else? |
|
The iOS error is a bug in master. I've seen it before, but I don't think there was an issue, so I filed #1616 to track it. |
|
I've made the requested change but one of the tests has failed without printing the reason. I think it might be just a network timeout - that test passes on my computer. It also did pass in the other build (on linux). |
There was a problem hiding this comment.
Thanks, sorry to be so picky about depsets.
I got bitten really hard by them last fall: we were taking like 15 minutes in analysis for kubectl I think. It turned out we had depsets within structs within depsets, and we were spending a ton of time hashing and de-duplicating. That caused us to split GoArchive into GoArchive and GoArchiveData (the latter is intended to be depset-friendly).
Since then, I get nervous when I see one.
|
I restarted Travis CI on macOS. I think I've seen that test flake in the past though, and I don't think it's because of this change. |
* Move legacy reproducibility test to new setup (#1585) * Move legacy reproducibility test to new setup Also included: 1. Adds an LLVM toolchain to the WORKSPACE file that can be used to manually test changes against clang on linux. 2. Adds os.PathSeparator to stripped absolute paths to be consistent with stripping being done everywhere else. 3. Adds a test to check the string "bazel-sandbox" in the binary. 4. Uses the cgo binary instead of a pure binary to check for reproducibility in the go_test. 5. Tags the target "collect_digests" as manual. To see how binaries are not reproducible with clang and `-g`, use ``` bazel test --copt=-g --crosstool_top=@llvm_toolchain//:toolchain \ //tests/reproducibility:go_default_test ``` * remove manual tag from reproducibility test * Do not use debug mode for reproducibility test. * Update bazel-toolchain to auto detect OS version * Add a primitive benchmark (#1599) bazel_benchmark checks out rules_go to a temporary directory, creates a temporary workspace, measures the time it takes to build various targets, then appends the times to a .csv file. This will probably be much more sophisticated in the future, but it's good to have something basic now. * go/tools/bazel_benchmark: extract some logic into bash script (#1601) bazel_benchmark.sh is now responsible for cloning rules_go at master into a temp directory. This script can be copied to a bin directory and run with a timer. The rest of the bazel_benchmark.go logic will run at the tip of master. Also: record Bazel version in the output file. * Set Bazel version tested in Travis CI to 0.15.0 (#1604) * Speed up downloading of @go_googleapis by using http_archive. (#1603) The googleapis repository seems to be of such a size that it takes a long time to clone on a link with lower bandwidth. So long, in fact, that it causes timeouts in my case. * Use add_all(), add_joined() instead of deprecated functionality of add() (#1602) * Change crosstool dependency to @bazel_tools//tools/cpp:current_cc_toolchain (#1605) //tools/defaults is a special case which is being removed in Bazel. * Add go_sdk rule and GoSDK provider (#1606) go_sdk is a new rule that gathers information about an SDK and returns a GoSDK provider which will get wired into the toolchain. package_list is a new rule that generates a list of importable packages from the sources in the SDK (previously, we invoked go list, which is slower). * Wire go_sdk and go_toolchain together (#1607) * go_toolchain has a new mandatory attribute, "sdk", which be something that provides GoSDK. * go_host_sdk, go_local_sdk, and go_download_sdk are now macros that wrap the old rules. Each rule declares toolchains in its BUILD.bazel file that work on the host architecture. The macro calls register_toolchains with these. * go_register_toolchains no longer calls register_toolchains, but it will an SDK rule if "go_sdk" isn't defined. This is a step toward allowing multiple SDKs to support multiple execution platforms. * Action inputs are narrowed to use go.sdk.tools and go.stdlib.libs rather than larger sets of files. * stdlib now uses precompiled libraries if the mode is compatible (#1608) * Remove deprecated --batch flag (#1617) * Add go_wrap_sdk rule (#1618) go_wrap_sdk allows you to configure a Go SDK that was downloaded or located with another repository rule. Related #1611 * Optimize args and inputs construction (#1610) * Use add_all() to lazily construct args * add_joined() omits the argument if value is an empty list * Optimize compile * Optimize cover * Simplify tags argument construction * Use any() instead of a dict * Undo depset() usage * Statically link tool binaries (#1615) * A few arguments construction cleanups (#1621) * Update toolchain and provider documentation [skip ci] (#1622) * Document race, msan and other attributes for go_binary, go_test [skip ci] (#1624) * Create .bazelrc (#1626) * Create .bazelrc See bazelbuild/bazel#5756 (comment) * Update .bazelrc * Remove explicit Label() construction (#1627) attr.label() converts strings into labels by itself. * Make go_sdk's package_list optional (#1625) This will eventually be removed when old versions of Gazelle are no longer supported and nothing depends on the file by name. If not provided as an input, go_sdk will generate the file itself. * Update deprecated single_file -> allow_single_file attribute (#1628) Also remove allow_files where it conflicts with allow_single_file (not allowed). Also remove both allow_files and allow_single_file in private attributes where executable is set to True (a file cannot be specified anyway - private attribute). * Document how to avoid proto conflicts [skip ci] (#1631) Fixes #1548 * Set RULES_GO_VERSION to 0.14.0 (#1633) * Propagate mode aspect on "_coverdata" edges (#1632) * Propagate mode aspect on "_coverdata" edges This ensures the coverdata library is built in the same mode as the binary that depends on it. Fixes #1630 * set pure = "on" on test to make CI happy * Update dependencies (#1634) bazel_gazelle to master as of 2018-08-06 com_google_protobuf to v3.6.1 com_github_goog_protobuf to v1.1.1 org_golang_x_net to master as of 2018-08-06 org_golang_google_grpc to v1.14.0 org_golang_google_genproto to master as of 2018-08-06 go_googleapis to master as of 2018-08-06 com_github_kevinburke_go_bindata to v3.11.0 org_golang_x_tools to master as of 2018-08-07 * Announce release 0.14.0 [skip ci] (#1636) Also, fix gazelle example to use prefix directive instead of attribute. * Add CI config to test on RBE. (#1638) * Add CI config to test on RBE. * Disable BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN. This is set by default in the rbe_ubuntu1604 platform, but tests in this repo need this to be disabled. * Skip tests that are not RBE compatible. * Use latest release version of bazel-toolchains repo. (#1639) * Declare org_golang_x_sys in go_rules_dependencies (#1649) The newest version of gRPC depends on @org_golang_x_sys. Since we provide other gRPC dependencies, we should declare this one as well. Fixes #1648 * Document how to override go_rules_dependencies [skip ci] (#1650) Related #1649 * Update minimum version of Bazel for Travis CI to 0.16.0 (#1651) Also, remove logic in .travis.yml for downloading Bazel at HEAD. We're not doing that anymore. * Actions that use go.args may now use param files automatically (#1652) Multiple param files are now supported as well. * Split go.args into go.builder_args and go.tool_args (#1653) Both helpers enable multiline files. Any action using either of these helpers should support them. Other actions may use go.actions.args. go.builder_args adds default arguments that builders should be able to interpret, including -sdk and -tags. go.args is deprecated. * Use -importcfg files for compiling and linking (#1654) The Go toolchain has supported importcfg files since 1.9. These files give the build system finer control over dependencies using importmap and packagefile declarations. Using these files allows us to abandon -I and -L flags, which will help us stay under command line length limits. Fixes #1637 * Update genproto dependencies (#1657) * update org_golang_google_genproto * update go_googleapis * Add test generates long compile/link command lines (#1655) Related #1637 * Remove 'cfg = "data"' from all attributes (#1658) The "data" configuration has been deprecated for a while and has no effect. * Remove gazelle and its deps from go_rules_dependencies (#1659) go_rules_dependencies no longer declares the following repositories: * bazel_gazelle * com_github_bazelbuild_buildtools * com_github_pelletier_go_toml The "gazelle" rule is removed from //go:def.bzl. It has been deprecated for some time, and "gazelle fix" replaces it. * Windows: Use absolute and shortened path for GOROOT environment variable (#1647) * Update tests ahead of Go 1.11 (#1661) A test in the old version of org_golang_x_crypto we were testing fails with Go 1.11. This is fixed in newer versions. * Remove uses of deprecated dictionary concatenation (#1663) This removes the need for --incompatible_disallow_dict_plus * Force absolute paths in builders (#1664) * Update org_golang_x_tools to master as of 2018-08-15 (#1662) * Set RULES_GO_VERSION to 0.15.0 (#1665) * Announce release 0.15.0 [skip ci] (#1666) * one character fix to README boilerplate [skip ci] (#1667) * doc: fix grammatical error (#1671)
Applied advice from https://docs.bazel.build/versions/master/skylark/performance.html to some of the files. WDYT?