Implicit HeaderMaps #3712

jerrymarino · 2017-09-09T02:24:27Z

Request for feedback on an implementation of C++ HeaderMaps in Bazel.

A HeaderMap is a data structure that allows the compiler to lookup included
headers in constant time.

Traditionally, the compiler has to parse a large string of iquote includes,
and then search these directories for a given header. This is slow for many
reasons.

The protocol of HeaderMap is implemented within compilers. Please find the
Lexer implementation in Clang.
https://clang.llvm.org/doxygen/HeaderMapTypes_8h.html
https://clang.llvm.org/doxygen/HeaderMap_8cpp_source.html

Use case:

I'm seeing a massive increase in build performance by using this. It cut my
clean build time in half.

Performance data:

Build time before HeaderMap:

Target //Pinterest/iOS/App:PinterestDevelopment up-to-date:
bazel-bin/Pinterest/iOS/App/PinterestDevelopment.ipa
____Elapsed time: 373.588s, Critical Path: 18.86s

Build time after header maps on the entire project:

Target //Pinterest/iOS/App:PinterestDevelopment up-to-date:
bazel-bin/Pinterest/iOS/App/PinterestDevelopment.ipa
____Elapsed time: 188.971s, Critical Path: 17.11s

Additionally, this solves the problem of having namespaced headers which is used
in CocoaPods all over. Using a namespace makes includes more clear since it is
easier for the user to distinguish where the header was derived.

Implementation:

At the ObjC level, headermaps are created with a namespace of the given target.
In objc_library it is possible for the user to override the value of the
namespace via the new attribute, header_namespace.

By using 2 headermaps the headersearchs are most efficient: a headermap for the
current target, and a header map with namespaced includes.

Users can include headers from ObjC targets in the convention of
Namespace/Header.h. Projects that don't use namespacing should see benefits as
well: includes of the form Header.h will be read from the headermap.

HeaderMapInfo contains all of the transitive info for dependent header maps,
and is merged together into a single map. This yields much better performance
than multiple headermaps.

This is my first PR to the Bazel repo, so any suggestions or feedback is greatly
appreciated!

bazel-io · 2017-09-09T02:24:29Z

Can one of the admins verify this patch?

philwo · 2017-09-11T12:31:20Z

@lberki @ulfjack Could you please take a look at this? Maybe Manuel would also be interested in this?

lberki · 2017-09-11T13:15:35Z

/cc @mhlopko

First the fundamentals:

How are these files passed to Clang? On the command line?
What happens if two header maps conflict or if what the header maps say is inconsistent with what the outcome header search would be?
How would this work with non-Clang compilers, e.g. gcc or MSVC?

Since these are important enough questions, I took the liberty of only skimming over the change, and there were two things that came to mind:

You seem to pass a specific string-to-string map to the new rule. It looks like it's easy to override anything the C++ compiler might want to include (e.g. <stdio.h>), which looks like a bit too much rope. Maybe a better approach is to generate these from the hdrs= declarations somehow (handwave-handwave)
It's true that having external utilities is a bit of an overhead, but what you get out of it is that you can change them without waiting for the next Bazel release. So all things considered, I'd prefer as much functionality as possible to be outside of the source of Bazel. In my experience, that makes for much less frustration.

jerrymarino · 2017-09-11T21:54:03Z

Thanks for the feedback @lberki!

How are these files passed to Clang? On the command line?

First, a HeaderMap is included in the hdrs attribute, which makes it a dependency of a given rule. A user passes the file to a compiler via include options: in Bazel via the includes and copts attributes. Additionally, it needs an include opt for the current directory -I .

Example:

header_map(
    name = "SomeHeaderMap",
    map = {
        "Header.h": "Some/Header.h"
    }
)

objc_library(
    name = "Some",
    hdrs = [
        "Some/Header.h",

        # Depend on and propagate the HeaderMap
        ":SomeHeaderMap"
    ],

    # include genfiles -> SomeHeaderMap.hmap
    includes = [
        "SomeHeaderMap.hmap",

        # Needed for HeaderMaps
        "-I", "."
    ]
)

What happens if two header maps conflict or if what the header maps say is inconsistent with what the outcome header search would be?

The compiler resolves conflicts internally. At a higher level, it is a similar issue if a user added conflicting include paths; Say, "Header.h" on disk in 2 different directories and both directories are isystem included. Similar to include order, there is a precedence order to HeaderMap includes.

How would this work with non-Clang compilers, e.g. gcc or MSVC?

HeaderMaps are supported on Apple's GCC Fork.

I'm not familiar with MSVC, but it looks like on WinObjc, they have support for HeaderMaps.

You seem to pass a specific string-to-string map to the new rule. It looks like it's easy to override anything the C++ compiler might want to include (e.g. <stdio.h>), which looks like a bit too much rope. Maybe a better approach is to generate these from the hdrs= declarations somehow (handwave-handwave)

I definitely think it should generate them based on hdrs ( perhaps in a similar fashion as modulemaps are ). For me, this would have netted a 2 minute decrease in clean build times, without any complexity on the my end! I can look into adding support for this, but it may be a larger change.

For some of my use cases, a custom map is required: I need to specify a custom namespace for headers. The namespace is not exposed to bazel objc_library rule in any way.

For example, in CocoaPods, headers are included from a given Pod with a namespace.

<AsyncDisplayKit/Header.h>

And on the file system:

external/*Texture*/Arbitrary/Path/To/Header.h

The string AsyncDisplayKit doesn't necessary exist at the rule or file system level, but other CocoaPods and consumers import the header with that namespace.

So for generating maps for CocoaPods or other Apple style libs, I think manually specifying namespaced headers is useful. Doing so at the BUILD file level seems less obtrusive and has the added benefit of being more flexible for other use cases.

It's true that having external utilities is a bit of an overhead, but what you get out of it is that you can change them without waiting for the next Bazel release. So all things considered, I'd prefer as much functionality as possible to be outside of the source of Bazel. In my experience, that makes for much less frustration.

Waiting for official Bazel releases isn't a big issue for me. At this point, I'm using custom builds for this and other changes.

r4nt · 2017-09-12T08:20:46Z

I agree this should be implemented very similarly to module maps - that is, automatically generated from the build rules. For namespacing, it sounds like we could add a hdrs_ns attribute that would set the mapped to namespace for a particularly library?

lberki · 2017-09-12T11:27:50Z

How does the compiler find the header map? As far as I can tell, this change doesn't change its command line. What happens if multiple header maps are supplied? Is there a precedence between them? If so, what is it?

I agree with @r4nt -- it looks like it's better to compute header maps from information already given to Bazel so that one doesn't run the risk of providing inconsistent information in the header_map build rule and in e.g. hdrs= attributes.

How stable are header maps? The links you provided are to Clang internals, which doesn't fill me with confidence that it will remain the same for a reasonable amount of time.

d0k · 2017-09-12T12:16:09Z

The header map format hasn't changed in the last 10 years and probably has been stable for a long time before that. However, it is undocumented.

It's essentially a hashmap to accelerate file system queries and thus replaces directory lookups with a path string lookup. Header maps are specified with -Ifoo.hmap on the command line and act pretty much the same as specifying a directory. You can have multiple of those flags or mix them with normal -Idir and they will be queried in command line order.

Automatically generating header maps to accelerate Clang build times (GCC and MSVC are unlikely to ever implement this) seems worth investigating. Getting the granularity right for that might be tricky though, you cannot represent include order within a single header map but making many small header maps destroys the potential compile time speedup.

thomasvl · 2017-09-12T12:29:38Z

Drive by - Why would you want this? Last time I looked, header maps (which were created by Xcode), they were keyed by the header name only. So if you have foo.h in two directories, it couldn't model it correctly and even thought you might have the property includes set up to control the search order, the compiler could end up using the wrong one because of what "won" in the header map.

jerrymarino · 2017-09-12T18:02:59Z

Thank you kindly for the consideration and feedback!

As @d0k mentioned, HeaderMaps have been around for a while. They have been default enabled in production Xcode under Clang and before that Apple GCC.

For my iOS app, the traditional approach of header lookup does not scale. I've got a large target and directory of ~1700 headers, a few small internal deps, and ~50 external deps ( CocoaPods ). Total, it puts me at ~4500 headers and respective directories.

The poor performance even at this scale is due to the nature of the compiler's header search algorithm. The complexity of linear searching over included directories is the issue here. Build times without this are a non starter even on the best Apple hardware.

Anecdotally, as an experiment, I replaced HeaderMaps with regular includes in Xcode and saw a 3x slowdown in Xcode's clean build time.

Ultimately, I think having a few maps at most for each compiler invocation ( and no other includes ) will be the fastest.

For a given target, I should be able to implicitly derive a headermap map based on hdrs which will boost performance across the board. The ideal headermap implementation for transitive headers is more nuanced and needs more consideration and testing.

I'm going to test a few different implementations to see how much build time I can regain. An implementation that just works would be ideal it's possible.

r4nt · 2017-09-13T07:09:05Z

One question is whether we should have a header map for compiling each target; that way, we have
a) a single header map -> max performance
b) full control over how the header map is structured according to the dependencies
c) creating the header map is an extra step that will take time in O(transitive deps), which is the downside

jerrymarino · 2017-09-16T01:14:23Z

So I've tested out a few different approaches to implementing this in of Bazel and the performance implications.

Header maps are created internally in Bazel and implicitly.

ObjC Namepace heuristic

Namespaced keys allow users to include headers from ObjC targets in the convention of <Namespace/Header.h> This is idiomatic for headers inclusions in ObjC development and CocoaPods. ( #306 )

By default headermaps are created with a namespace based on the name of a given target.

In objc_library it is possible for the user to override the value of the namespace via the new attribute, header_namespace.

Dependency headers and namespaces are added transitively with a TransitiveInfoProvider.

From the BUILD file perspective, this is easier than having to create a header_map manually. It wouldn't be possible to achieve this level of performance or clean API with a Skylark based rule.

Performance: Cuts clean build time in half

By using 2 headermaps the header searches are most efficient: a small -iquote included headermap for the current target, and an -I included headermap. Additionally, Bazel processes smaller compiler invocations now: all of the includes I previously needed are reduced to 2 includes.

Clean build time on base commit:

Target //Pinterest/iOS/App:PinterestDevelopment up-to-date:
bazel-bin/Pinterest/iOS/App/PinterestDevelopment.ipa
____Elapsed time: 373.588s, Critical Path: 18.86s

Clean build time after implicit header maps:

Target //Pinterest/iOS/App:PinterestDevelopment up-to-date:
bazel-bin/Pinterest/iOS/App/PinterestDevelopment.ipa
____Elapsed time: 188.971s, Critical Path: 17.11s

Overall, I think it's a good improvement 🎉

dmishe · 2017-09-16T03:49:55Z

<> usually are used for system header imports. I understand that header maps gave access to user imports using system syntax, but the latest Xcode seems to do away with this practice:

The traditional header map which was generated when the “Always Search User Paths”
(ALWAYS_SEARCH_USER_PATHS) setting was YES is not supported by the new build system.
Instead, projects should set ALWAYS_SEARCH_USER_PATHS to NO and should migrate to
using modern header include syntax:

Use quote-style include ("foo.h") for project headers, and reserve angle-bracket include
(<foo.h>) for system headers.

I think this is a good change, and makes your imports clear. Can header maps be used with "" includes instead?

jerrymarino · 2017-09-18T17:59:24Z

@dmishe thanks for the feedback. On this branch, the header search routine will find imports of the form: "Namespace/Header.h" from the implicit header maps. Existing imports of "Header.h"from deps will work too. I think you may see some speedups just by using this branch on existing repos.

jerrymarino · 2017-09-19T20:15:22Z

I've updated the patch based on the above discussions: added a couple tests and removed the original rule. The implicit header map logic handles all use cases I could think of.

I also wrapped it behind an option experimental_enable_implicit_headermaps, which is defaulted to off. It shouldn't impact builds in any way that don't have it set.

cc @mhlopko @lberki @ulfjack Is there any other feedback that you have for this?

jerrymarino · 2017-09-19T20:21:15Z

src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java

+        "   hdrs = ['headerb.h'],",
+        ")");
+
+    assertThat(getConfiguredTarget("//x:objc")).isNotNull();


I didn't see a way to actually build //x:objc here, but that could be good to have it running end to end!

The Java tests are not integration tests. You'd need to write an integration test to do an actual build.

@ulfjack thanks for the feedback. I'm not too familiar with the integration testing system yet but will look into this.

Well the easiest would be to take inspiration from our existing integration tests, e.g. https://github.com/bazelbuild/bazel/blob/master/src/test/shell/integration/cpp_test.sh

jerrymarino · 2017-09-25T18:56:24Z

@mhlopko friendly ping! Would you kindly take another look at this?

If there anything you all think can be done here to get this in a mergeable state, I'm happy to discuss 👍

hlopko

I didn't have time to finish the review today, so sending at least partial review out. One more note - please do not make it objc specific, it's useful for c++ too. And as you'll see in the comments, pls integrate it with include_prefix and strip_include_prefix.

hlopko · 2017-09-26T10:16:19Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java

+      headerMapsBuilder.add(internalHeaderMap);
+
+      String namespace;
+      if (ruleContext.attributes().has("header_namespace")) {


Could we use include_prefix and strip_include_prefix for this? So we don't need another attribute.

And for "this" I mean header_namespace attribute.

I could take a stab at making it work with the existing include_prefix and strip_include_prefix attributes in cc_library too.

None the less, I think the behavior of header_namespace is useful for objc_library and especially objc that was written under Xcode's conventions.

The current implementation of objc_library -> header_namespace has quite different abilities than cc_library -> include_prefix / cc_library -> strip_include_prefix. Basically, header_namespace puts every single header under a flat namespace regardless of that header's location on disk.

Even if objc_library gained the include_prefix and strip_include_prefix attributes, I don't think it'd be trivial to setup includes for the following repo:

LibA BUILD A1 SRC1.c LibB BUILD C1 HDR1.h C2 HDR2.h D1 HDR3.h .. continues for tens of levels of headers

File LibA/BUILD

objc_library( name = "LibA", srcs = ["A1/SRC1.c"], deps = ["//LibB:LibB"], )

File LibA/A1/SRC1.c

// The header file resides at LibB/C1/D1/HDR3.h #import "B/HDR3.h"

File LibB/BUILD

objc_library( name = "LibB" hdrs = ["C1/HDR1.h", "C2/HDR2.h", "C2/D1/HDR3.h"], # Every single header is available under namespace "B" # i.e. #include "B/HDR3.h" header_namespace = "B" )

When we compile LibA/SRC1.c we are passed the following module map:

B/HDR1.h -> LibB/C1/HDR1.h B/HDR2.h -> LibB/C2/HDR2.h B/HDR2.h -> LibB/C2/D1/HDR2.h ... more

So includes in File SRC1.c are valid

// The header file resides at LibB/C1/D1/HDR3.h #include "B/HDR3.h"

In CocoaPods.org and other iOS development, this include style is widely used

Given the usage of includes in iOS development ( i.e. in Texture ) and Xcode's include behavior I think it makes sense for objc_library to have the header_namespace attribute.

I think I argued for something like header_namespace back in the day; I do think you bring good arguments why it provides an easier user experience. Given that, it might make sense to also support this for C++, as well as supporting it both with and without header maps.

Thanks for the feedback @r4nt! For me, in addition to perf benefits, this feature key to making Xcode based source compatible with Bazel in a way that's not to much effort too.

If this is can be upstreamed, then I should be able to add header_namespace to C++ library as well.

Ideally, getting header_namespace to work without header maps could be broken out into a separate PR. I'm not exactly sure what that would entail, but it seems doable.

Does that sound like a good plan @mhlopko?

Is the "flattening" behavior of header_namespace really something people rely on? It sounds confusing and error prone to me because of header name collisions. In order to implement this without header map (because people might want to use header_namespace even when they don't want header maps) we would have to pass -Idir for every dir in the target. I need more convincing that this is good idea @jerrymarino @r4nt :) If we remove "flattening" requirement, then header_namespace behaves exactly like include_prefix. Is there any way to collect more data on how often people rely of the "flattening"?

If we want to go into the direction of allowing OS code to work together without changes, it's fundamentally similar to the include_prefix and strip_include_prefix, while being simpler to understand and providing more flexibility, so I'd say it's a clear win.

hlopko · 2017-09-26T10:19:22Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java

+      ImmutableList.Builder<Artifact> headerMapsBuilder = ImmutableList.builder();
+      String targetName = ruleContext.getTarget().getName();
+
+      HeaderMapInfo.Builder internalHeaderMapInfo = new HeaderMapInfo.Builder();


Also include virtual headers for include_prefix/strip_include_prefix

hlopko · 2017-09-26T10:21:54Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java

+      depHeaderMapInfo.addHeaders(publicTextualHeaders);
+
+      // Merge all of the header map info from deps. The headers within a given
+      // target have precedence over over dep headers ( See


SuperNit: extra space between ( and See.

hlopko · 2017-09-26T10:28:26Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java

+      // the working directory ( i.e. exec root ) in this form
+      // and it must be after including the header map files
+      contextBuilder.addIncludeDir(PathFragment.create("."));
+	    ImmutableList headerMaps = headerMapsBuilder.build();


SuperNit: Reformat pls. In general, Bazel follows google java style guide (https://google.github.io/styleguide/javaguide.html). I'll take care of that when importing, but to increase readability pls try to adhere to column limit: 100, extra points for https://google.github.io/styleguide/javaguide.html#s4.5.1-line-wrapping-where-to-break :) But I'm nitting.

Cool, thanks for the tip. I'll try to make this follow to the style guide closely :)

hlopko · 2017-09-26T10:29:14Z

src/main/java/com/google/devtools/build/lib/rules/cpp/ClangHeaderMap.java

+public final class ClangHeaderMap {
+  // Logical representation of a bucket.
+  // The actual data is stored in the string pool.
+  private class HMapBucket {


Nit: Let's make this comment a javadoc.

And consider renaming to HeaderMapBucket?

Sure, I'll rename it to HeaderMapBucket

hlopko · 2017-09-26T10:51:47Z