Skip to content

Commit

Permalink
Improve performance and output of bazel_determinism_test.
Browse files Browse the repository at this point in the history
Don't ask me how so many things can be wrong in a single test...

Progress towards #4770.

FYI @rupertks @buchgr @ulfjack

## Replace 25000 invocations of perl with a single "sha256sum"

This speeds up the test by a factor 2x on my iMac (before: 1200s, now: 600s).

On macOS, "shasum" is a Perl script. Instead of simply passing all input files to the thing at once, we were invoking it once per file. This means roughly 25,000 invocations of Perl per test run. And it's even worse - it wasn't just a call to that Perl script, it was wrapped in a "cat | shasum | cut" pipeline, resulting in silent data loss when you accidentally passed multiple input files to the thing, 75,000 processes being spawned just to compute hashes and losing the file name of what was actually hashed. WTF.

Also, we were using SHA256 to essentially verify that two directory trees are equal. For this purpose, relying on SHA1 should be absolutely fine - and that is, provided by a good native implementation, four times faster than `shasum`. It saves another 10 seconds of the overall run.

With this change, the test also prints the result of a failed determinism check in an easier to read format "filename hash" instead of "hash filename" and on top of that, it also prints the filenames in the diff on macOS, which was missing formerly. Without this, it was basically impossible to debug failures of this test on macOS, as you couldn't see *which files were different*. You had *one* job, bazel_determinism_test.

Before:

```
-- Test log: -----------------------------------------------------------
--- /private/var/tmp/_bazel_buildkite/30004132848cb6cbb0d8bc124cd9712b/bazel-sandbox/8820973750646175047/execroot/io_bazel/_tmp/e503f3f3df14b71e247bc3d7d9bf3608/sum1	2018-03-28 18:00:43.000000000 +0000
+++ /private/var/tmp/_bazel_buildkite/30004132848cb6cbb0d8bc124cd9712b/bazel-sandbox/8820973750646175047/execroot/io_bazel/_tmp/e503f3f3df14b71e247bc3d7d9bf3608/sum2	2018-03-28 18:10:34.000000000 +0000
@@ -10417,0 +10418 @@
+ecd53ba69a8d479d3fa4234e959f869cd10f7ebc68860d2b7915879f8b8b2c54
@@ -10605 +10605,0 @@
-f1954b59039b74d0a0ee3b2bced748604b95b8455a5bf80489296bd81878a5c8
------------------------------------------------------------------------
```

Now (I artificially introduced non-hermeticism to show how a failure would look like):

```
-- Test log: -----------------------------------------------------------
--- /private/var/tmp/_bazel_philwo/7a01905b4627ca044e5e3f5ad5b14d26/bazel-sandbox/5464595340038418595/execroot/io_bazel/_tmp/e503f3f3df14b71e247bc3d7d9bf3608/sum1 2018-03-30 17:12:39.000000000 +0000
+++ /private/var/tmp/_bazel_philwo/7a01905b4627ca044e5e3f5ad5b14d26/bazel-sandbox/5464595340038418595/execroot/io_bazel/_tmp/e503f3f3df14b71e247bc3d7d9bf3608/sum2 2018-03-30 17:17:27.000000000 +0000
@@ -903 +903 @@
-bazel-bin/src/bazel 31d811338ca364f0631560dd4d29406dd6a778ce
+bazel-bin/src/bazel 8f009173894730b00a1d1d6349af7d10f4d21cf3
@@ -5656 +5656 @@
-bazel-bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar f5ec8c4415ad8ecdc0385affc68f2dd4dbf241ef
+bazel-bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar 9899ae35cf431087a34a830bfdaf19d99616689c
@@ -8343 +8343 @@
-bazel-bin/src/main/java/com/google/devtools/build/lib/worker/_javac/worker/libworker_classes/com/google/devtools/build/lib/worker/WorkerFactory.class 780baa17c19ef99ef0b9291db1791ed8e0f1b231
+bazel-bin/src/main/java/com/google/devtools/build/lib/worker/_javac/worker/libworker_classes/com/google/devtools/build/lib/worker/WorkerFactory.class d45c14f09e73e7fcdf01f96aa32646c87b704bc2
@@ -8359 +8359 @@
-bazel-bin/src/main/java/com/google/devtools/build/lib/worker/libworker.jar 60e3afbfec17da7e44c1f0f61cf2a446196717be
+bazel-bin/src/main/java/com/google/devtools/build/lib/worker/libworker.jar 70f557e87d1b32b2e46c79554fe6bf3b89aeaf6e
@@ -11343 +11343 @@
-bazel-genfiles/src/install_base_key 3fad754e4ea19bd1120df5bf16e1f39372e6b9fe
+bazel-genfiles/src/install_base_key 7d7e8b62493912c5ec153032e104640e3980e6b3
@@ -11376 +11376 @@
-bazel-genfiles/src/package.zip 1ce3431b021ca338806162eca72ff84118001df5
+bazel-genfiles/src/package.zip 65f4801d91bbe10cba0d2d4d55c7cf319cd6722d
------------------------------------------------------------------------
test_determinism FAILED: Non-deterministic outputs found! .
```

## Remove obsolete check for BAZEL_TEST_XTRACE

That string does not appear anywhere in our repo, except for these two lines in the test, so there's no point in checking for it.

## Remove obsolete check for Java 7

That was about time.

## Performance improvements and usability fixes

- There's no need to use mktemp to create a unique directory under TEST_TMPDIR, as every test suite has its own TEST_TMPDIR.
- There's no need to remove stuff, as this will just degrade performance and make debugging harder. The surrounding Bazel or system will clean up later.
- There's no need to copy bazel-bin/src/bazel to ./bazel1 before calling it, as you can just call the built bazel from its original location.
- There's no need to run "bazel clean" before the second "bazel build" invocation - it's better to just use two separate output_bases. This is faster and also makes debugging easier, as you can compare the two output_bases in case of a test failure.
- There's no need to call "diff" twice - we can just save the output immediately in the `if` block.

Closes #4945.

PiperOrigin-RevId: 191118833
  • Loading branch information
philwo authored and Copybara-Service committed Mar 30, 2018
1 parent 7a5761c commit b5e893a
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 35 deletions.
66 changes: 33 additions & 33 deletions src/test/shell/bazel/bazel_determinism_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -22,49 +22,49 @@ set -u
DISTFILE=$(rlocation io_bazel/${1#./})
shift 1

if [ "${JAVA_VERSION:-}" == "1.7" ] ; then
echo "Warning: bootstrapping not tested for java 1.7"
exit 0
fi

# Load the test setup defined in the parent directory
source $(rlocation io_bazel/src/test/shell/integration_test_setup.sh) \
|| { echo "integration_test_setup.sh not found!" >&2; exit 1; }

# sha1sum is easily twice as fast as shasum, but not always available on macOS.
if hash sha1sum 2>/dev/null; then
shasum="sha1sum"
elif hash shasum 2>/dev/null; then
shasum="shasum"
else
fail "Could not find sha1sum or shasum on the PATH"
fi

function hash_outputs() {
[ -n "${BAZEL_TEST_XTRACE:-}" ] && set +x # Avoid garbage in the output
# runfiles/MANIFEST & runfiles_manifest contain absolute path, ignore.
# ar on OS-X is non-deterministic, ignore .a files.
for i in $(find bazel-bin/ -type f -a \! -name MANIFEST -a \! -name '*.runfiles_manifest' -a \! -name '*.a'); do
sha256sum $i
done
for i in $(find bazel-genfiles/ -type f); do
sha256sum $i
done
[ -n "${BAZEL_TEST_XTRACE:-}" ] && set -x
}

function get_outputs_sum() {
hash_outputs | sort -k 2
find bazel-bin/ bazel-genfiles/ \
-type f \
-a \! -name MANIFEST \
-a \! -name '*.runfiles_manifest' \
-a \! -name '*.a' \
-exec $shasum {} + \
| awk '{print $2, $1}' \
| sort \
| sed "s://:/:"
}

function test_determinism() {
local olddir=$(pwd)
WRKDIR=$(mktemp -d ${TEST_TMPDIR}/bazelbootstrap.XXXXXXXX)
mkdir -p "${WRKDIR}" || fail "Could not create workdir"
trap "rm -rf \"$WRKDIR\"" EXIT
cd "${WRKDIR}" || fail "Could not change to work directory"
unzip -q ${DISTFILE}
# Compile bazel a first time
bazel build --nostamp src:bazel
cp bazel-bin/src/bazel bazel1
get_outputs_sum >"${TEST_TMPDIR}/sum1"
./bazel1 clean --expunge
./bazel1 build --nostamp src:bazel
get_outputs_sum >"${TEST_TMPDIR}/sum2"
if ! (diff -q "${TEST_TMPDIR}/sum1" "${TEST_TMPDIR}/sum2"); then
diff -U0 "${TEST_TMPDIR}/sum1" "${TEST_TMPDIR}/sum2" >$TEST_log
fail "Non deterministic outputs found!"
local workdir="${TEST_TMPDIR}/workdir"
mkdir "${workdir}" || fail "Could not create work directory"
cd "${workdir}" || fail "Could not change to work directory"
unzip -q "${DISTFILE}"

# Build Bazel once.
bazel --output_base="${TEST_TMPDIR}/out1" build --nostamp //src:bazel
hash_outputs >"${TEST_TMPDIR}/sum1"

# Build Bazel twice.
bazel-bin/src/bazel --output_base="${TEST_TMPDIR}/out2" build --nostamp //src:bazel
hash_outputs >"${TEST_TMPDIR}/sum2"

if ! diff -U0 "${TEST_TMPDIR}/sum1" "${TEST_TMPDIR}/sum2" >$TEST_log; then
fail "Non-deterministic outputs found!"
fi
}

Expand Down
4 changes: 2 additions & 2 deletions src/test/shell/testenv.sh
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,11 @@ exit 1;
case "${PLATFORM}" in
darwin|freebsd)
function sha256sum() {
cat "$1" | shasum -a 256 | cut -f 1 -d " "
shasum -a 256 "$@"
}
;;
*)
# Under linux sha256sum should exists
# Under Linux, sha256sum usually exists as a binary as part of coreutils.
;;
esac

Expand Down

0 comments on commit b5e893a

Please sign in to comment.