Add performance and best practices documentation

Test: check gitiles view
Change-Id: I2fa4fa5f7ee91ba586e1900542c8c334eb727a6b

3 files changed, 343 insertions, 0 deletions

diff --git a/docs/best_practices.md b/docs/best_practices.md
new file mode 100644
index 00000000..546e19e2
--- /dev/null
+++ b/docs/best_practices.md
@@ -0,0 +1,148 @@
+# Build System Best Practices
+
+## Read only source tree
+
+Never write to the source directory during the build, always write to
+`$OUT_DIR`. We expect to enforce this in the future.
+
+If you want to verify / provide an update to a checked in generated source
+file, generate that file into `$OUT_DIR` during the build, fail the build
+asking the user to run a command (either a straight command, checked in script,
+generated script, etc) to explicitly copy that file from the output into the
+source tree.
+
+## Network access
+
+Never access the network during the build. We expect to enforce this in the
+future, though there will be some level of exceptions for tools like `distcc`
+and `goma`.
+
+## Paths
+
+Don't use absolute paths in Ninja files (with make's `$(abspath)` or similar),
+as that could trigger extra rebuilds when a source directory is moved.
+
+Assume that the source directory is `$PWD`. If a script is going to change
+directories and needs to convert an input from a relative to absolute path,
+prefer to do that in the script.
+
+Don't encode absolute paths in build intermediates or outputs. This would make
+it difficult to reproduce builds on other machines.
+
+Don't assume that `$OUT_DIR` is `out`. The source and output trees are very
+large these days, so some people put these on different disks. There are many
+other uses as well.
+
+Don't assume that `$OUT_DIR` is under `$PWD`, users can set it to a relative path
+or an absolute path.
+
+## $(shell) use in Android.mk files
+
+Don't use `$(shell)` to write files, create symlinks, etc. We expect to
+enforce this in the future. Encode these as build rules in the build graph
+instead.  This can be problematic in a number of ways:
+
+* `$(shell)` calls run at the beginning of every build, at minimum this slows
+  down build startup, but it can also trigger more build steps to run than are
+  necessary, since these files will change more often than necessary.
+* It's no longer possible for a stripped-down product configuration to opt-out
+  of these created files. It's better to have actual rules and dependencies set
+  up so that space isn't wasted, but the files are there when necessary.
+
+## Headers
+
+`LOCAL_COPY_HEADERS` is deprecated. Soong modules cannot use these headers, and
+when the VNDK is enabled, System modules in Make cannot declare or use them
+either.
+
+The set of global include paths provided by the build system is also being
+removed. They've been switched from using `-isystem` to `-I` already, and are
+removed entirely in some environments (vendor code when the VNDK is enabled).
+
+Instead, use `LOCAL_EXPORT_C_INCLUDE_DIRS`/`export_include_dirs`. These allow
+access to the headers automatically if you link to the associated code.
+
+If your library uses `LOCAL_EXPORT_C_INCLUDE_DIRS`/`export_include_dirs`, and
+the exported headers reference a library that you link to, use
+`LOCAL_EXPORT_SHARED_LIBRARY_HEADERS`/`LOCAL_EXPORT_STATIC_LIBRARY_HEADERS`/`LOCAL_EXPORT_HEADER_LIBRARY_HEADERS`
+(`export_shared_lib_headers`/`export_static_lib_headers`/`export_header_lib_headers`)
+to re-export the necessary headers to your users.
+
+Don't use non-local paths in your `LOCAL_EXPORT_C_INCLUDE_DIRS`, use one of the
+`LOCAL_EXPORT_*_HEADERS` instead. Non-local exported include dirs are not
+supported in Soong. You may need to either move your module definition up a
+directory (for example, if you have ./src/ and ./include/, you probably want to
+define the module in ./Android.bp, not ./src/Android.bp), define a header
+library and re-export it, or move the headers into a more appropriate location.
+
+Prefer to use header libraries (`BUILD_HEADER_LIBRARY`/ `cc_library_headers`)
+only if the headers are actually standalone, and do not have associated code.
+Sometimes there are headers that have header-only sections, but also define
+interfaces to a library. Prefer to split those header-only sections out to a
+separate header-only library containing only the header-only sections, and
+re-export that header library from the existing library. This will prevent
+accidentally linking more code than you need (slower at build and/or runtime),
+or accidentally not linking to a library that's actually necessary.
+
+Prefer `LOCAL_EXPORT_C_INCLUDE_DIRS` over `LOCAL_C_INCLUDES` as well.
+Eventually we'd like to remove `LOCAL_C_INCLUDES`, though significant cleanup
+will be required first. This will be necessary to detect cases where modules
+are using headers that shouldn't be available to them -- usually due to the
+lack of ABI/API guarantees, but for various other reasons as well: layering
+violations, planned deprecations, potential optimizations like C++ modules,
+etc.
+
+## Use defaults over variables
+
+Soong supports variable definitions in Android.bp files, but in many cases,
+it's better to use defaults modules like `cc_defaults`, `java_defaults`, etc.
+
+* It moves more information next to the values -- that the array of strings
+  will be used as a list of sources is useful, both for humans and automated
+  tools.  This is even more useful if it's used inside an architecture or
+  target specific property.
+* It can collect multiple pieces of information together into logical
+  inheritable groups that can be selected with a single property.
+
+## Custom build tools
+
+If writing multiple files from a tool, declare them all in the build graph.
+* Make: Use `.KATI_IMPLICIT_OUTPUTS`
+* Android.bp: Just add them to the `out` list in genrule
+* Custom Soong Plugin: Add to `Outputs` or `ImplicitOutputs`
+
+Declare all files read by the tool, either with a dependency if you can, or by
+writing a dependency file. Ninja supports a fairly limited set of dependency
+file formats. You can verify that the dependencies are read correctly with:
+
+```
+NINJA_ARGS="-t deps <output_file>" m
+```
+
+Prefer to list input files on the command line, otherwise we may not know to
+re-run your command when a new input file is added. Ninja does not treat a
+change in dependencies as something that would invalidate an action -- the
+command line would need to change, or one of the inputs would need to be newer
+than the output file. If you don't include the inputs in your command line, you
+may need to add the the directories to your dependency list or dependency file,
+so that any additions or removals from those directories would trigger your
+tool to be re-run. That can be more expensive than necessary though, since many
+editors will write temporary files into the same directory, so changing a
+README could trigger the directory's timestamp to be updated.
+
+Only control output files based on the command line, not by an input file. We
+need to know which files will be created before any inputs are read, since we
+generate the entire build graph before reading source files, or running your
+tool. This comes up with Java based tools fairly often -- they'll generate
+different output files based on the classes declared in their input files.
+We've worked around these tools with the "srcjar" concept, which is just a jar
+file containing the generated sources. Our Java compilation tasks understand
+*.srcjar files, and will extract them before passing them on to the compiler.
+
+## Libraries in PRODUCT_PACKAGES
+
+Most libraries aren't necessary to include in `PRODUCT_PACKAGES`, unless
+they're used dynamically via `dlopen`. If they're only used via
+`LOCAL_SHARED_LIBRARIES` / `shared_libs`, then those dependencies will trigger
+them to be installed when necessary. Adding unnecessary libraries into
+`PRODUCT_PACKAGES` will force them to always be installed, wasting space.
diff --git a/docs/perf.md b/docs/perf.md
new file mode 100644
index 00000000..c3a26474
--- /dev/null
+++ b/docs/perf.md
@@ -0,0 +1,195 @@
+# Build Performance
+
+## Debugging Build Performance
+
+### Tracing
+
+soong_ui has tracing built in, so that every build execution's trace can be
+viewed.  Just open `$OUT_DIR/build.trace.gz` in Chrome's <chrome://tracing>, or
+with [catapult's trace viewer][catapult trace_viewer]. The last few traces are
+stored in `build.trace.#.gz` (larger numbers are older). The associated logs
+are stored in `soong.#.log`.
+
+![trace example](./trace_example.png)
+
+### Soong
+
+Soong can be traced and profiled using the standard Go tools. It understands
+the `-cpuprofile`, `-trace`, and `-memprofile` command line arguments, but we
+don't currently have an easy way to enable them in the context of a full build.
+
+### Kati
+
+In general, the slow path of reading Android.mk files isn't particularly
+performance sensitive, since it doesn't need to happen on every build. It is
+important for the fast-path (detecting whether it needs to regenerate the ninja
+file) to be fast however. And it shouldn't hit the slow path too often -- so
+don't rely on output of a `$(shell)` command that includes the current timestamp,
+or read a file that's going to change on every build.
+
+#### Regen check is slow
+
+In most cases, we've found that the fast-path is slow because all of the
+`$(shell)` commands need to be re-executed to determine if their output changed.
+The `$OUT_DIR/soong.log` contains statistics from the regen check:
+
+```
+.../kati.go:127: *kati*: regen check time: 1.699207
+.../kati.go:127: *kati*: glob time (regen): 0.377193 / 33609
+.../kati.go:127: *kati*: shell time (regen): 1.313529 / 184
+.../kati.go:127: *kati*:  0.217 find device vendor -type f -name \*.pk8 -o -name verifiedboot\* -o -name \*.x509.pem -o -name oem\*.prop | sort
+.../kati.go:127: *kati*:  0.105 cd packages/apps/Dialer ; find -L . -type d -name "res"
+.../kati.go:127: *kati*:  0.035 find device vendor -maxdepth 4 -name '*_aux_variant_config.mk' -o -name '*_aux_os_config.mk' | sort
+.../kati.go:127: *kati*:  0.029 cd frameworks/base ; find -L  core/java graphics/java location/java media/java media/mca/effect/java media/mca/filterfw/java media/mca/filterpacks/java drm/java opengl/java sax/java telecomm/java telephony/java wifi/java lowpan/java keystore/java rs/java ../opt/telephony/src/java/android/telephony ../opt/telephony/src/java/android/telephony/gsm ../opt/net/voip/src/java/android/net/rtp ../opt/net/voip/src/java/android/net/sip   -name "*.html" -and -not -name ".*"
+.../kati.go:127: *kati*:  0.025 test -d device && find -L device -maxdepth 4 -path '*/marlin/BoardConfig.mk'
+.../kati.go:127: *kati*:  0.023 find packages/apps/Settings/tests/robotests -type f -name '*Test.java' | sed -e 's!.*\(com/google.*Test\)\.java!\1!' -e 's!.*\(com/android.*Test\)\.java!\1!' | sed 's!/!\.!g' | cat
+.../kati.go:127: *kati*:  0.022 test -d vendor && find -L vendor -maxdepth 4 -path '*/marlin/BoardConfig.mk'
+.../kati.go:127: *kati*:  0.017 cd cts/tests/tests/shortcutmanager/packages/launchermanifest ; find -L  ../src -name "*.java" -and -not -name ".*"
+.../kati.go:127: *kati*:  0.016 cd cts/tests/tests/shortcutmanager/packages/launchermanifest ; find -L  ../../common/src -name "*.java" -and -not -name ".*"
+.../kati.go:127: *kati*:  0.015 cd libcore && (find luni/src/test/java -name "*.java" 2> /dev/null) | grep -v -f java_tests_blacklist
+.../kati.go:127: *kati*: stat time (regen): 0.250384 / 4405
+```
+
+In this case, the total time spent checking was 1.69 seconds, even though the
+other "(regen)" numbers add up to more than that (some parts are parallelized
+where possible). The biggest contributor is the `$(shell)` times -- 184
+executions took a total of 1.31 seconds. The top 10 longest shell functions are
+printed.
+
+All the longest commands in this case are all variants of a call to `find`, but
+this is where using pure make functions instead of calling out to the shell can
+make a performance impact -- many calls to check if `26 > 20` can add up. We've
+added some basic math functions in `math.mk` to help some common use cases that
+used to be rather expensive when they were used too often.
+
+There are some optimizations in place for find commands -- if Kati can
+understand the find command, the built-in find emulator can turn some of them
+into glob or stat checks (falling back to calling `find` if one of those imply
+that the output may change). Many of the common macros produce find commands
+that Kati can understand, but if you're writing your own, you may want to
+experiment with other options if they're showing up in this list. For example,
+if this was significantly more expensive (either in runtime, or was called
+often):
+
+```
+.../kati.go:127: *kati*:  0.015 cd libcore && (find luni/src/test/java -name "*.java" 2> /dev/null) | grep -v -f java_tests_blacklist
+```
+
+It may be more efficient to move the grep into make, so that the `find` portion
+can be rewritten and cached:
+
+```
+$(filter-out $(file <$(LOCAL_PATH)/java_tests_blacklist),$(call all-java-files-under,luni/src/test/java))
+```
+
+Others can be simplified by just switching to an equivalent find command that
+Kati understands:
+
+```
+.../kati.go:127: *kati*:  0.217 find device vendor -type f -name \*.pk8 -o -name verifiedboot\* -o -name \*.x509.pem -o -name oem\*.prop | sort
+```
+
+By adding the implicit `-a` and moving the `| sort` to Make, this can now be
+cached by Kati:
+
+```
+$(sort $(shell find device vendor -type -f -a -name \*.pk8 -o -name verifiedboot\* -o -name \*.x509.pem -o -name oem\*.prop))
+```
+
+Kati is learning about the implicit `-a` in [this change](https://github.com/google/kati/pull/132)
+
+#### Kati regens too often
+
+Kati prints out what triggered the slow path to be taken -- this can be a
+changed file, a changed environment variable, or different output from a
+`$(shell)` command:
+
+```
+out/soong/Android-aosp_arm.mk was modified, regenerating...
+```
+
+The state is stored in `$OUT_DIR/.kati_stamp*` files, and can be (partially)
+read with the `ckati_stamp_dump` tool in prebuilts/build-tools. More debugging
+is available when ckati is run with `--regen_debug`, but that can be a lot of
+data to understand.
+
+### Ninja
+
+#### Understanding why something rebuilt
+
+Add `NINJA_ARGS="-d explain"` to your environment before a build, this will cause
+ninja to print out explanations on why actions were taken. Start reading from the
+beginning, as this much data can be hard to read:
+
+```
+$ cd art
+$ mma
+$ touch runtime/jit/profile_compilation_info.h
+$ NINJA_ARGS="-d explain" mma
+...
+ninja explain: output out/soong/.intermediates/art/tools/cpp-define-generator/cpp-define-generator-data/linux_glibc_x86_64/obj/art/tools/cpp-define-generator/main.o older than most recent input art/runtime/jit/profile_compilation_info.h (
+1516683538 vs 1516685188)
+ninja explain: out/soong/.intermediates/art/tools/cpp-define-generator/cpp-define-generator-data/linux_glibc_x86_64/obj/art/tools/cpp-define-generator/main.o is dirty
+ninja explain: out/soong/.intermediates/art/tools/cpp-define-generator/cpp-define-generator-data/linux_glibc_x86_64/cpp-define-generator-data is dirty
+ninja explain: out/soong/host/linux-x86/bin/cpp-define-generator-data is dirty
+ninja explain: out/soong/.intermediates/art/tools/cpp-define-generator/cpp-define-generator-asm-support/gen/asm_support_gen.h is dirty
+ninja explain: out/soong/.intermediates/art/cmdline/art_cmdline_tests/android_arm_armv7-a_core_cmdline_parser_test/obj/art/cmdline/cmdline_parser_test.o is dirty
+...
+```
+
+In this case, art/cmdline/cmdline_parser_test.o was rebuilt because it uses
+asm_support_gen.h, which was generated by cpp-define-generator-data, which uses
+profile_compilation_info.h.
+
+You'll likely need to cross-reference this data against the build graph in the
+various .ninja files. The files are (mostly) human-readable, but a (slow) web
+interface can be used by running `NINJA_ARGS="-t browse <target>" m`.
+
+#### Builds take a long time
+
+If the long part in the trace view of a build is a relatively solid block, then
+the performance is probably more related to how much time the actual build
+commands are taking than having extra dependencies, or slowdowns in
+soong/kati/ninja themselves.
+
+Beyond looking at visible outliers in the trace view, we don't have any tooling
+to help in this area yet. It's possible to aggregate some of the raw data
+together, but since our builds are heavily parallelized, it's particularly easy
+for build commands to impact unrelated build commands. This is an area we'd
+like to improve -- we expect keeping track of user/system time per-action would
+provide more reliable data, but tracking some full-system data (memory/swap
+use, disk bandwidth, etc) may also be necessary.
+
+## Known Issues
+
+### Common
+
+#### mm
+
+Soong always loads the entire module graph, so as modules convert from Make to
+Soong, `mm` is becoming closer to `mma`. This produces more correct builds, but
+does slow down builds, as we need to verify/produce/load a larger build graph.
+
+We're exploring a few options to speed up build startup, one being [an
+experimental set of ninja patches][ninja parse optimization],
+though that's not the current path we're working towards.
+
+### Android 8.1 (Oreo MR1)
+
+In some cases, a tree would get into a state where Soong would be run twice on
+every incremental build, even if there was nothing to do. This was fixed in
+master with [these changes][blueprint_microfactory], but they were too
+significant to backport at the time. And while they fix this particular issue,
+they appear to cause ninja to spend more time during every build loading the
+`.ninja_log` / `.ninja_deps` files, especially as they become larger.
+
+A workaround to get out of this state is to remove the build.ninja entry from
+`$OUT_DIR/.ninja_log`:
+
+```
+sed -i "/\/build.ninja/d" $(get_build_var OUT_DIR)/.ninja_log
+```
+
+[catapult trace_viewer]: https://github.com/catapult-project/catapult/blob/master/tracing/README.md
+[ninja parse optimization]: https://android-review.googlesource.com/c/platform/external/ninja/+/461005
+[blueprint_microfactory]: https://android-review.googlesource.com/q/topic:%22blueprint_microfactory%22+status:merged
diff --git a/docs/trace_example.png b/docs/trace_example.png
new file mode 100644
index 00000000..d3d12d7e
--- /dev/null
+++ b/docs/trace_example.png