[clang][deps] Cherry-pick the 1st batch of performance improvements #7206
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch moves `llvm::sort()` from `AnalyzerOptions` constructor to initialization of local static variable in `isUnknownAnalyzerConfig()`. This avoids unnecessary work, which can speed up Clang tools that initialize lots of `CompilerInvocation`s (and therefore `AnalyzerOptions`). Reviewed By: steakhal Differential Revision: https://reviews.llvm.org/D137258 (cherry picked from commit 53a4a2b)
Clang currently updates the mtime of .timestamp files on each load of the corresponding .pcm file. This is not necessary. In a given build session, Clang only needs to write the .timestamp file once, when we first validate the input files. This patch makes it so that we only touch the .timestamp file when it's older than the build session, alleviating some filesystem contention in clang-scan-deps. Reviewed By: benlangmuir Differential Revision: https://reviews.llvm.org/D149802 (cherry picked from commit 63eb04a)
In 9c25418 `ASTWriter` stopped writing identifiers that are not interesting. Taking it a bit further, we don't need to sort the whole identifier table, just the interesting identifiers. This reduces the size of sorted vector from ~10k (including lots of builtins) to 2 (`__VA_ARGS__` and `__VA_OPT__`) in a typical Xcode project, improving `clang-scan-deps` performance. Reviewed By: benlangmuir Differential Revision: https://reviews.llvm.org/D150494 (cherry picked from commit f60cc47)
In `clang-scan-deps` contexts, the number of interesting identifiers in PCM files is fairly low (only macros), while the number of identifiers in the importing instance is high (builtins). Marking the whole identifier table out-of-date triggers lots of benign and expensive calls to `ASTReader::updateOutOfDateIdentifiers()`. (That unfortunately happens even for unused identifiers due to `SemaRef.IdResolver.begin(II)` line in `ASTWriter::WriteASTCore()`.) This patch makes the main code path more similar to C++ modules, where the PCM files have `INTERESTING_IDENTIFIERS` section which lists identifiers that get created in the identifier table of the importing instance and marked as out-of-date. The only difference is that the main code path doesn't *create* identifiers in the table and relies on the importing instance calling `ASTReader::get()` when creating new identifier on-demand. It only marks existing identifiers as out-of-date. This speeds up `clang-scan-deps` by 5-10%. Reviewed By: Bigcheese, benlangmuir Differential Revision: https://reviews.llvm.org/D151277 (cherry picked from commit c68ba12)
In D114095, `HeaderFileInfo::NumIncludes` was moved into `Preprocessor`. This still makes sense, because we want to track this on the granularity of submodules (D112915, D114173), but the way this information is serialized is not ideal. In `ASTWriter`, the set of included files gets deserialized eagerly, issuing lots of calls to `FileManager::getFile()` for input files the PCM consumer might not be interested in. This patch makes the information part of the header file info table, taking advantage of its lazy deserialization which typically happens when a file is about to be included. Reviewed By: benlangmuir Differential Revision: https://reviews.llvm.org/D155131 (cherry picked from commit 6504d87)
This is a prep patch for avoiding the quadratic number of calls to `HeaderSearch::lookupModule()` in `ASTReader` for each (transitively) loaded PCM file. (Specifically in the context of `clang-scan-deps`). This patch explicitly serializes `Module::DefinitionLoc` so that we can stop relying on it being filled by the module map parser. This change also required change to the module map parser, where we used the absence of `DefinitionLoc` to determine whether a file came from a PCM file. We also need to make sure we consider the "containing" module map affecting when writing a PCM, so that it's not stripped during serialization, which ensures `DefinitionLoc` still ends up pointing to the correct offset. This is intended to be a NFC change. Reviewed By: benlangmuir Differential Revision: https://reviews.llvm.org/D150292 (cherry picked from commit abcf7ce)
Currently, `ASTReader` performs some checks to diagnose relocated modules. This can add quite a bit of overhead to the scanner: it requires looking up, parsing and resolving module maps for all transitively loaded module files (and all the module maps encountered in the search paths on the way). Most of those checks are not really useful in the scanner anyway, since it uses strict context hash and immutable filesystem, which prevent those scenarios in the first place. This can speed up scanning by up to 30%. Depends on D150292. Reviewed By: benlangmuir Differential Revision: https://reviews.llvm.org/D150320 (cherry picked from commit 227f719)
…ng "FW" PCM When Clang loads a PCM that depends on another PCM describing framework module "FW", `ModuleMap` registers "FW" as known, without seeing the module map that defines it (or the adjacent "FW_Private" module map). Later, when looking at a header from "FW_Private", `ModuleMap` returns early due to having knowledge about "FW" and never associates that header with "FW_Private", leading to it being treated as textual. This behavior is caused by D150292, where the scanner stops calling `HeaderSearch::lookupModule()` eagerly for every loaded PCM. This patch skips an early check when trying to figure out the framework module for a header, which ensures the "FW" and (most importantly) "FW_Private" module maps can be parsed even after loading "FW" from a PCM. Note that the `HeaderSearch::loadModuleMapFile()` function we not call unconditionally has caching behavior of its own, meaning it will avoid parsing module map file repeatedly. Depends on D150320. Reviewed By: benlangmuir Differential Revision: https://reviews.llvm.org/D150478 (cherry picked from commit be01456)
…maps Before D150478, there were situations when Clang avoided parsing a module map because it was likely to re-define an already defined module (either by a PCM or by previously-found module map). Since Clang no longer performs that check and does parse the extra module map (due to the FW/FW_Private issue described in D150478), this patch re-implements the same semantics by skipping the duplicate definition of the framework module while parsing the module map. Depends on D150478. Reviewed By: benlangmuir Differential Revision: https://reviews.llvm.org/D150479 (cherry picked from commit dba2b5c)
(cherry picked from commit 2c20ea4)
…iles into a PCH Clang writes the set of textually included files into AST files, so that importers know to avoid including those files again and instead deserialize their contents from the AST on-demand. Logic for determining the set of included files files only considers headers that are either non-modular or that are modular but with `HeaderFileInfo::isCompilingModuleHeader` set. Logic for computing that bit is different than the one that determines whether to include a header textually with the "-fmodule-name=Mod" option. That can lead to header from module "Mod" being included textually in a PCH, but be omitted in the serialized set of included files. This can then allow such header to be textually included from importer of the PCH, wreaking havoc. This patch fixes that by aligning the logic for computing `HeaderFileInfo::isCompilingModuleHeader` with the logic for deciding whether to include modular header textually. As far as I can tell, this bug has been in Clang for forever. It got accidentally "fixed" by D114095 (that changed the logic for determining the set of included files) and got broken again in D155131 (which is essentially a revert of the former). rdar://113520515 Reviewed By: benlangmuir Differential Revision: https://reviews.llvm.org/D157559 (cherry picked from commit bbdb0c7)
|
@swift-ci please test |
|
Please also cherry-pick this to |
Please cherry-pick even if it will not affect API in swift. :) |
No, only things that are absolutely necessary to unblock rebranch qualification should be merged into this branch. |
jansvoboda11
deleted the
jan_svoboda/stable-20221013-scanner-perf-cherry-pick
branch
|
I feel like there's a bit of confusion happening here. To be clear, stable/20230725 is branched off of next. That happened on July 25th (hence the branch name). So only commits after that point need to be cherry-picked onto stable/20230725. That's maybe some of these, but I doubt it's all.
While I would love this to be the case, it's just going to cause problems. If a change is being cherry-picked to stable/20221013 and it isn't on stable/20230725, it should also be cherry-picked to stable/20230725. Doing just API changes is going to cause trouble down the road that isn't worth it, including conflicts that wouldn't otherwise happen. Having said that, please do run testing to check that (1) the cherry-pick builds and (2) there aren't any new test failures caused by it. It's also likely much easier to do the cherry-pick from the next branch, since we really should not be that far diverged at this point (I would hope). It will have also dealt with any merge conflicts from apple-fork only changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.