[PATCH] D80681: [clang][SourceManager] cache Macro Expansions

Wed May 27 18:02:23 PDT 2020

nickdesaulniers created this revision.
nickdesaulniers added reviewers: rsmith, bkramer.
Herald added subscribers: cfe-commits, jfb, kristof.beyls.
Herald added a project: clang.

A seemingly innocuous Linux kernel change [0] seemingly blew up our
compile times by over 3x, as reported by @nathanchance in [1].

The code in question uses a doubly nested macro containing GNU C
statement expressions that are then passed to typeof(), which is then
used in a very important macro for atomic variable access throughout
most of the kernel. The inner most macro, is passed a GNU C statement
expression.  In this case, we have macro arguments that are GNU C
statement expressions, which can contain a significant number of tokens.
The upstream kernel patch caused significant build time regressions for
both Clang and GCC. Since then, some of the nesting has been removed via
@melver, which helps gain back most of the lost compilation time. [2]

Profiles collected [3] from compilations of the slowest TU for us in the
kernel show:

- 51.4% time spent in clang::TokenLexer::updateLocForMacroArgTokens
- 48.7% time spent in clang::SourceManager::getFileIDLocal
- 35.5% time spent in clang::SourceManager::isOffsetInFileID

(mostly calls from the former through to the latter).

So it seems we have a pathological case for which properly tracking the
SourceLocation of macro arguments is significantly harming build
performance. This stands out in referenced flame graph.

In fact, this case was identified previously as being problematic in
commit 3339c568c4 ("[Lex] Speed up updateConsecutiveMacroArgTokens (NFC)")

Looking at the above call chain, there's 3 things we can do to speed up
this case.

1. TokenLexer::updateConsecutiveMacroArgTokens() calls SourceManager::isWrittenInSameFile() which calls SourceManager::getFileID(), which is both very hot and very expensive to call. SourceManger has a one entry cache, member LastFileIDLookup. If that isn't the FileID for a give source location offset, we fall back to a linear probe, and then to a binary search for the FileID. These fallbacks update the one entry cache, but noticeably they do not for the case of macro expansions!

  For the slowest TU to compile in the Linux kernel, it seems that we miss about 78.67% of the 68 million queries we make to getFileIDLocal that we could have had cache hits for, had we saved the macro expansion source location's FileID in the one entry cache. [4]

  I tried adding a separate cache item for macro expansions, and to check that before the linear then binary search fallbacks, but did not find it faster than simply allowing macro expansions into the one item cache.  This alone nets us back a lot of the performance loss.

  That said, this is a modification of caching logic, which is playing with a double edged sword.  While it significantly improves the pathological case, its hard to say that there's not an equal but opposite pathological case that isn't regressed by this change. Though non-pathological cases of builds of the Linux kernel before [0] are only slightly improved (<1%) and builds of LLVM itself don't change due to this patch.

  Should future travelers find this change to significantly harm their build times, I encourage them to feel empowered to revert this change.

2. SourceManager::getFileIDLocal has a FIXME hinting that the call to SourceManager::isOffsetInFileID could be made much faster since isOffsetInFileID is generic in the sense that it tries to handle the more generic case of "local" (as opposed to "loaded") files, though the caller has already determined the file to be local. This patch implements a new method that specialized for use when the caller already knows the file is local, then use that in TokenLexer::updateLocForMacroArgTokens.  This should be less controversial than 1, and is likely an across the board win. It's much less significant for the pathological case, but still a measurable win once we have fallen to the final case of binary search.

3. A bunch of methods in SourceManager take a default argument. SourceManager::getLocalSLocEntry doesn't do anything with this argument, yet many callers of getLocalSLocEntry setup, pass, then check this argument. This is wasted work. We can't remove the parameter outright, as it's part of an interface expected by getInclusions() in libclang.

With this patch applied, the above profile [5] for the same pathological
input looks like:

- 25.1% time spent in clang::TokenLexer::updateLocForMacroArgTokens
- 17.2% time spent in clang::SourceManager::getFileIDLocal

and clang::SourceManager::isOffsetInFileID is no longer called, and thus
falls out of the profile.

There may be further improvements to the general problem of "what
interval contains one number out of millions" than the current use of a
one item cache, followed by linear probing, followed by binary
searching. We might even be able to do something smarter in
TokenLexer::updateLocForMacroArgTokens.

[0] https://github.com/ClangBuiltLinux/linux/commit/cdd28ad2d8110099e43527e96d059c5639809680
[1] https://github.com/ClangBuiltLinux/linux/issues/1032
[2] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=locking/kcsan&id=a5dead405f6be1fb80555bdcb77c406bf133fdc8
[3] https://github.com/ClangBuiltLinux/linux/issues/1032#issuecomment-633712667
[4] https://github.com/ClangBuiltLinux/linux/issues/1032#issuecomment-633741923
[5] https://github.com/ClangBuiltLinux/linux/issues/1032#issuecomment-634932736

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80681

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/lib/Basic/SourceManager.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D80681.266711.patch
Type: text/x-patch
Size: 4509 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200528/ee7e0cd5/attachment.bin>