bazel: Add pre-compiled clang header poc

[keith]

This adds a custom rule for producing clang pre-compiled headers. The
hope is that by using these we reduce significant duplication in some
larger headers that get recompiled by many dependees.

This works by fetching the normal C++ configuration from bazel's
crosstool, and adding custom flags to compile our pch in the right mode.

This currently has a few limitations / rough edges:

1. It only supports clang
2. There's no way with the CcInfo provider to propagate compiler flags
   up the dependency tree. Because of this we instead add the required
   -include-pch flag to all macros.
3. Pch files are timestamp sensitive, this means if you add a include to
   the pch, build, then remove it and rebuild, it will currently fail.
   LLVM master has a flag to ignore this timestamp, but it isn't
   currently available in the release version of Xcode on macOS.

Signed-off-by: Keith Smiley keithbsmiley@gmail.com

[lizan]

Do we have any data re: how this improve the overall build performance locally?

[ggreenway]

Do we have any data re: how this improve the overall build performance locally?

I'd be willing to do some tests around that. @keith is this in a good enough state for perf testing yet? Is this clang-only, or will it work with gcc?

[keith]

It would definitely be great if someone more familiar with these issues could test some configurations and see how it looks.

I could look into making it work with gcc as well, off the top of my head I'm not sure how much work that would be, but it might be best if we could prove it out with clang first assuming the performance characteristics would be the same?

I think this is ready for some testing, but definitely still in the alpha state since I haven't tried many configurations (and CI makes it look like there's an issue on non clang or maybe non macOS builds already).

[ggreenway]

I tried this and get error

ERROR: source/extensions/quic_listeners/quiche/platform/BUILD:95:17: C++ compilation of rule '//source/extensions/quic_listeners/quiche/platform:quic_platform_logging_impl_lib' failed (Exit 1)
error: unable to read PCH file bazel-out/k8-fastbuild/bin/source/common/common/common_pch.pch: 'No such file or directory'
fatal error: PCH file 'bazel-out/k8-fastbuild/bin/source/common/common/common_pch.pch' not found: module file not found
2 errors generated.

Also, I had to make the visibility of the test pch public to fix an bazel error in quic.

[keith]

What target were you building? And what platform?

[ggreenway]

linux: bazel build --config=clang-pch //source/...

[keith]

I'll try to repro in docker

[keith]

Pushed fixes for both of those, sorry for the trouble

[ggreenway]

This version builds, but everytime I rebuild (with no changes since the previous build), it rebuilds v8, which takes forever, and makes any timing measurements useless. Executing genrule @com_googlesource_chromium_v8//:build;

[keith]

Interesting. Based on bazel query I see no connections between that target and the pch targets. Would you expect their to be one? Does this query return anything in your configuration?

bazel query 'somepath(@com_googlesource_chromium_v8//..., //source/common/common:common_pch)'

[keith]

I removed no-cache here for now, I'm not sure I would expect that to help, but it's worth a try

[ggreenway]

$ bazel query 'somepath(@com_googlesource_chromium_v8//..., //source/common/common:common_pch)'
INFO: Empty results
Loading: 5 packages loaded

[keith]

Hrm. I wouldn't expect this to cause that then, but you don't see it without it?

[ggreenway]

Hrm. I wouldn't expect this to cause that then, but you don't see it without it?

Yep, I double-checked that going back to master it does nothing on rebuild when nothing has changed. No idea why this change causes v8 to rebuild.

[keith]

Thanks for testing. I'll debug early next week

[keith]

So I actually see this without this change, but only when sandboxing is disabled. It seems like the issue is when the build artifacts are left around the output is not hermetic. Specifically for example some files like the .ninja_deps files are included (which I assume contains timestamps):

@@ -7598,7 +7598,7 @@
 inputs {
   path: "external/com_googlesource_chromium_v8/wee8/out/wee8/.ninja_deps"
   digest {
-    hash: "a378893aa515feadcf3ce240040f076e17778f607507fd348a9e94d9093d8960"
+    hash: "6c83d26de24732e0cf89c8c882968f4d65feae9ccd5a91ea22d5db2110c3597f"
     size_bytes: 1199916
     hash_function_name: "SHA-256"
   }
@@ -7606,8 +7606,8 @@
 inputs {
   path: "external/com_googlesource_chromium_v8/wee8/out/wee8/.ninja_log"
   digest {
-    hash: "4f54dc5aa0dbb71d836b63a9a430ccca3fcdbc7b9123c6d25c2dde5c06d8fdd1"
-    size_bytes: 119886
+    hash: "6c7a83ad170f7331a3c3b149b94ede9aacce2cc1dd0f45f2f2cf9d5f54caaa84"
+    size_bytes: 119928
     hash_function_name: "SHA-256"
   }
 }

I'll debug this some more.

[keith]

Fix for that is here: #13866

[keith]

@ggreenway mind taking a pass at this again?

[ggreenway]

I got some results. To test, I first did a full build, then timed how long it took to build //source/exe/... after making a comment change in buffer.h. Results were consistent down to +- 1 second across multiple runs.

PCH took 7:17 to rebuild.
Baseline (non-PCH) took 8:12.

So that's a pretty good gain, considering we haven't even added a lot of stuff to the PCH yet. I think the next step is to add some of the protobuf generated headers to the PCH and see if we can get more gains.

[ggreenway]

I suppose one more sanity test is to benchmark without-pch, and without sandboxing, to make sure the improvement isn't due to the sandboxing. Do you know a bazel invocation that will do that off the top of your head?

[keith]

You can delete line 51 from the .bazelrc from this change to do that. Or copy the spawn strategy from line 50 and an exclude this new config from the command line

[ggreenway]

7:53 to rebuild with --spawn_strategy=local

[mattklein123]

I still think the biggest bang for the buck we will get is the mocks. Have you tried that yet?

[ggreenway]

I'm trying, but bazel is unhappy. I tried adding a mock header to the test pch, and to do that, I had to add the target as a dependency (to access the header), but that makes a cycle in the dependencies. @keith any idea how to resolve?

ERROR: /Users/ggreenway/dev/envoy/upstream/test/BUILD:41:18: in pch rule //test:test_pch: cycle in dependency graph:
    //test/test_common:contention_lib
.-> //test:test_pch
|   //test:test_pch_libs
|   //test/mocks/access_log:access_log_mocks
`-- //test:test_pch
This cycle occurred because of a configuration option
ERROR: Analysis of target '//test/test_common:contention_lib' failed; build aborted

envoy_pch_library(
    name = "test_pch",
    testonly = True,
    external_deps = [
        "googletest",
    ],
    includes = [
        "gmock/gmock.h",
        "gtest/gtest.h",
        "test/mocks/access_log/mocks.h",
    ],
    visibility = ["//visibility:public"],
    deps = [
        "//test/mocks/access_log:access_log_mocks",
    ],
)

[keith]

My assumption (without being at my computer) is that the mocks use the bazel macro where I implicitly added the pch as a dep, leading to that cycle. There's some UX question here of how we want to deal with that. For a quick workaround (assuming the dependency tree doesn't go deeper) you could exclude the pch in the macro if the name matches the target you're trying to add. I can work on a long term improvement here though.

[keith]

I won't have time to fix this up until next week

[keith]

Sorry I didn't get to this this week, I'm out for the holiday next week but it's on my list for afterwards. In the meantime if anyone has ideas on the UX question above they'd be appreciated!

[keith]

Still not sure entirely on what we should do for this case, but for now I've made all mocks not depend on the pch, so the pch can therefore depend on all / any mocks. I've added the one from your previous cycle error as an initial dependency for testing, lmk what you think!

[alyssawilk]

looks like this is failing DCO?

/wait

[keith]

ugh, I edited that in the github UI, thanks fixed

[keith]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13788 (comment) was created by @keith.

see: more, trace.

[keith]

@lizan when you get a chance can you re-review?

[mattklein123]

Thanks let's ship and iterate.

[keith]

Thanks everyone!