Page MenuHomePhabricator

[Preprocessor] Make the special lexing for dependency scanning a first-class feature of the `Preprocessor` and `Lexer`
ClosedPublic

Authored by akyrtzi on May 12 2022, 11:05 AM.

Details

Summary

Depends on D125487

This is 4/4 of a series of patches, bringing the following benefits:

  • Full access to the preprocessor state during dependency scanning. E.g. a component can see what includes were taken and where they were located in the actual sources.
  • Improved performance for dependency scanning. Measurements with a release+thin-LTO build shows ~ -11% reduction in wall time.
  • Opportunity to use dependency scanning lexing to speed-up skipping of excluded conditional blocks during normal preprocessing (as follow-up, not part of this patch).

For normal preprocessing measurements show differences are below the noise level.

Since, after this change, we don't minimize sources and pass them in place of the real sources, DependencyScanningFilesystem is not technically necessary, but it has valuable performance benefits for caching file stats along with the results of scanning the sources. So the setup of using the DependencyScanningFilesystem during a dependency scan remains.

Diff Detail

Unit TestsFailed

TimeTest
20,400 msx64 debian > Clangd Unit Tests._/ClangdTests/CompletionTest::DocumentationFromChangedFileCrash
Script: -- /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/clangd/unittests/./ClangdTests --gtest_filter=CompletionTest.DocumentationFromChangedFileCrash

Event Timeline

akyrtzi created this revision.May 12 2022, 11:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2022, 11:05 AM
akyrtzi requested review of this revision.May 12 2022, 11:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2022, 11:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
akyrtzi updated this revision to Diff 429033.May 12 2022, 11:35 AM

Added

#include "clang/Basic/FileEntry.h"

in PreprocessorOptions.h to accommodate the modules build.

Awesome to see this! Looking forward to the next step (using this in normal preprocessing!).

after this change, we don't minimize sources and pass them in place of the real sources

Is there code in DepFS that can/should be deleted as part of this patch, or in a follow-up, or is it still around as an option?

Is there code in DepFS that can/should be deleted as part of this patch, or in a follow-up, or is it still around as an option?

After these changes, with DepFS we are using its multi-threading sharding technique to cache file stats and the source file directive scanning results. We could use multi-threading sharding only to cache source file directive scanning results, and get rid of DepFS altogether, but then I don't see a good way to cache the file stats as well, unless we want to try to re-use the FileManager across depscan instances and make its stat caching as efficient as DepFS (maybe by generalizing the multi-threading sharding technique and using it in FileManager).

There's also FileSystemStatCache which seems like a leftover right now, but we could enhance it and have it shared by individual FileManager instances (instead of sharing the same FileManager in depscan instances).

What do you think?

Is there code in DepFS that can/should be deleted as part of this patch, or in a follow-up, or is it still around as an option?

[To be clear, my question was because I don't see this patch deleting the code path that minimizes / saves-minimized sources. Can/should we delete the "minimize sources" code path?]

After these changes, with DepFS we are using its multi-threading sharding technique to cache file stats and the source file directive scanning results. We could use multi-threading sharding only to cache source file directive scanning results, and get rid of DepFS altogether, but then I don't see a good way to cache the file stats as well, unless we want to try to re-use the FileManager across depscan instances and make its stat caching as efficient as DepFS (maybe by generalizing the multi-threading sharding technique and using it in FileManager).

There's also FileSystemStatCache which seems like a leftover right now, but we could enhance it and have it shared by individual FileManager instances (instead of sharing the same FileManager in depscan instances).

What do you think?

FWIW, I don't think we should be sharing FileManager at all, since it has state that makes sharing it unsound.

I like the direction of trying to remove FSStatCache. I think stat/content caching belongs at the VFS level so DepFS seems like a better starting point (maybe generalized a bit). Note that DepFS isn't just amortizing stat cost, it's also avoiding reopening / mmaping / mempcying files.

[To be clear, my question was because I don't see this patch deleting the code path that minimizes / saves-minimized sources. Can/should we delete the "minimize sources" code path?]

Oh, this is removed in the prior patch in the review stack (https://reviews.llvm.org/D125487)

I like the direction of trying to remove FSStatCache. I think stat/content caching belongs at the VFS level so DepFS seems like a better starting point (maybe generalized a bit). Note that DepFS isn't just amortizing stat cost, it's also avoiding reopening / mmaping / mempcying files.

That's a good point, so maybe the direction to go is to generalize the DepFS caching mechanism.

[To be clear, my question was because I don't see this patch deleting the code path that minimizes / saves-minimized sources. Can/should we delete the "minimize sources" code path?]

Oh, this is removed in the prior patch in the review stack (https://reviews.llvm.org/D125487)

Seems unfortunate to have a temporary regression in the commit stack, since then you can't push incrementally (or bisect). Can the prior patch leave behind the feature in the DependencyFilesystem, and this patch delete it now that clang-scan-deps doesn't depend on it for performance? (Or ignore me if I'm still not understanding...)

Seems unfortunate to have a temporary regression in the commit stack, since then you can't push incrementally (or bisect). Can the prior patch leave behind the feature in the DependencyFilesystem, and this patch delete it now that clang-scan-deps doesn't depend on it for performance? (Or ignore me if I'm still not understanding...)

I think creating an intermediate commit state where it's both doing lexing for tokens and source minimization is not worth the trouble.
When committing I could merge the last 2 patches together into one commit (or all 4 of them into a commit), so there's no commit state where depscan performance is regressed.
I mostly separated them like this for reviewing convenience.

akyrtzi updated this revision to Diff 431151.May 21 2022, 10:54 AM

Update due to source change in the previous patch (https://reviews.llvm.org/D125487)

This revision is now accepted and ready to land.May 23 2022, 5:24 AM

Thank you @jansvoboda11 for reviewing and helping me qualify the changes! 🙇🏻‍♂️

If I receive no further comments I will commit the changes on Thursday; if anyone wants some more time to take a look just let me know.
I intend to commit in 2 commits, the first patch as one commit and the next three as a second commit; this is to ensure there's no commit where there's a performance regression in dependency scanning.