[clang-tools-extra] r338012 - [clangd] Do not rebuild AST if inputs have not changed
Ilya Biryukov via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 26 02:21:08 PDT 2018
Author: ibiryukov
Date: Thu Jul 26 02:21:07 2018
New Revision: 338012
URL: http://llvm.org/viewvc/llvm-project?rev=338012&view=rev
Log:
[clangd] Do not rebuild AST if inputs have not changed
Summary:
If the contents are the same, the update most likely comes from the
fact that compile commands were invalidated. In that case we want to
avoid rebuilds in case the compile commands are actually the same.
Reviewers: ioeric
Reviewed By: ioeric
Subscribers: simark, javed.absar, MaskRay, jkorous, arphaman, jfb, cfe-commits
Differential Revision: https://reviews.llvm.org/D49783
Modified:
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/test/clangd/extra-flags.test
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
clang-tools-extra/trunk/unittests/clangd/TestFS.cpp
clang-tools-extra/trunk/unittests/clangd/TestFS.h
Modified: clang-tools-extra/trunk/clangd/TUScheduler.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.cpp?rev=338012&r1=338011&r2=338012&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp (original)
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp Thu Jul 26 02:21:07 2018
@@ -324,6 +324,11 @@ void ASTWorker::update(
ParseInputs Inputs, WantDiagnostics WantDiags,
llvm::unique_function<void(std::vector<Diag>)> OnUpdated) {
auto Task = [=](decltype(OnUpdated) OnUpdated) mutable {
+ // Will be used to check if we can avoid rebuilding the AST.
+ bool InputsAreTheSame =
+ std::tie(FileInputs.CompileCommand, FileInputs.Contents) ==
+ std::tie(Inputs.CompileCommand, Inputs.Contents);
+
tooling::CompileCommand OldCommand = std::move(FileInputs.CompileCommand);
FileInputs = Inputs;
// Remove the old AST if it's still in cache.
@@ -343,16 +348,38 @@ void ASTWorker::update(
return;
}
- std::shared_ptr<const PreambleData> NewPreamble = buildPreamble(
- FileName, *Invocation, getPossiblyStalePreamble(), OldCommand, Inputs,
- PCHs, StorePreambleInMemory, PreambleCallback);
+ std::shared_ptr<const PreambleData> OldPreamble =
+ getPossiblyStalePreamble();
+ std::shared_ptr<const PreambleData> NewPreamble =
+ buildPreamble(FileName, *Invocation, OldPreamble, OldCommand, Inputs,
+ PCHs, StorePreambleInMemory, PreambleCallback);
+
+ bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
{
std::lock_guard<std::mutex> Lock(Mutex);
if (NewPreamble)
LastBuiltPreamble = NewPreamble;
}
+ // Before doing the expensive AST reparse, we want to release our reference
+ // to the old preamble, so it can be freed if there are no other references
+ // to it.
+ OldPreamble.reset();
PreambleWasBuilt.notify();
+ if (CanReuseAST) {
+ // Take a shortcut and don't build the AST, neither the inputs nor the
+ // preamble have changed.
+ // Note that we do not report the diagnostics, since they should not have
+ // changed either. All the clients should handle the lack of OnUpdated()
+ // call anyway, to handle empty result from buildAST.
+ // FIXME(ibiryukov): the AST could actually change if non-preamble
+ // includes changed, but we choose to ignore it.
+ // FIXME(ibiryukov): should we refresh the cache in IdleASTs for the
+ // current file at this point?
+ log("Skipping rebuild of the AST for {0}, inputs are the same.",
+ FileName);
+ return;
+ }
// Build the AST for diagnostics.
llvm::Optional<ParsedAST> AST =
buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs);
Modified: clang-tools-extra/trunk/test/clangd/extra-flags.test
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/extra-flags.test?rev=338012&r1=338011&r2=338012&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clangd/extra-flags.test (original)
+++ clang-tools-extra/trunk/test/clangd/extra-flags.test Thu Jul 26 02:21:07 2018
@@ -23,7 +23,7 @@
# CHECK-NEXT: "uri": "file://{{.*}}/foo.c"
# CHECK-NEXT: }
---
-{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///foo.c","version":2},"contentChanges":[{"text":"int main() { int i; return i; }"}]}}
+{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///foo.c","version":2},"contentChanges":[{"text":"int main() { int i; return i+1; }"}]}}
# CHECK: "method": "textDocument/publishDiagnostics",
# CHECK-NEXT: "params": {
# CHECK-NEXT: "diagnostics": [
Modified: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp?rev=338012&r1=338011&r2=338012&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Thu Jul 26 02:21:07 2018
@@ -33,13 +33,12 @@ void ignoreError(llvm::Error Err) {
class TUSchedulerTests : public ::testing::Test {
protected:
ParseInputs getInputs(PathRef File, std::string Contents) {
- return ParseInputs{*CDB.getCompileCommand(File), buildTestFS(Files),
- std::move(Contents)};
+ return ParseInputs{*CDB.getCompileCommand(File),
+ buildTestFS(Files, Timestamps), std::move(Contents)};
}
llvm::StringMap<std::string> Files;
-
-private:
+ llvm::StringMap<time_t> Timestamps;
MockCompilationDatabase CDB;
};
@@ -263,6 +262,10 @@ TEST_F(TUSchedulerTests, EvictedAST) {
int* a;
double* b = a;
)cpp";
+ llvm::StringLiteral OtherSourceContents = R"cpp(
+ int* a;
+ double* b = a + 0;
+ )cpp";
auto Foo = testPath("foo.cpp");
auto Bar = testPath("bar.cpp");
@@ -288,7 +291,7 @@ TEST_F(TUSchedulerTests, EvictedAST) {
ASSERT_THAT(S.getFilesWithCachedAST(), UnorderedElementsAre(Bar, Baz));
// Access the old file again.
- S.update(Foo, getInputs(Foo, SourceContents), WantDiagnostics::Yes,
+ S.update(Foo, getInputs(Foo, OtherSourceContents), WantDiagnostics::Yes,
[&BuiltASTCounter](std::vector<Diag> Diags) { ++BuiltASTCounter; });
ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
ASSERT_EQ(BuiltASTCounter.load(), 4);
@@ -334,5 +337,58 @@ TEST_F(TUSchedulerTests, RunWaitsForPrea
ASSERT_THAT(Preambles, Each(Preambles[0]));
}
+TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
+ TUScheduler S(
+ /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
+ /*StorePreambleInMemory=*/true, PreambleParsedCallback(),
+ /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ ASTRetentionPolicy());
+
+ auto Source = testPath("foo.cpp");
+ auto Header = testPath("foo.h");
+
+ Files[Header] = "int a;";
+ Timestamps[Header] = time_t(0);
+
+ auto SourceContents = R"cpp(
+ #include "foo.h"
+ int b = a;
+ )cpp";
+
+ // Return value indicates if the updated callback was received.
+ auto DoUpdate = [&](ParseInputs Inputs) -> bool {
+ std::atomic<bool> Updated(false);
+ Updated = false;
+ S.update(Source, std::move(Inputs), WantDiagnostics::Yes,
+ [&Updated](std::vector<Diag>) { Updated = true; });
+ bool UpdateFinished = S.blockUntilIdle(timeoutSeconds(1));
+ if (!UpdateFinished)
+ ADD_FAILURE() << "Updated has not finished in one second. Threading bug?";
+ return Updated;
+ };
+
+ // Test that subsequent updates with the same inputs do not cause rebuilds.
+ ASSERT_TRUE(DoUpdate(getInputs(Source, SourceContents)));
+ ASSERT_FALSE(DoUpdate(getInputs(Source, SourceContents)));
+
+ // Update to a header should cause a rebuild, though.
+ Files[Header] = time_t(1);
+ ASSERT_TRUE(DoUpdate(getInputs(Source, SourceContents)));
+ ASSERT_FALSE(DoUpdate(getInputs(Source, SourceContents)));
+
+ // Update to the contents should cause a rebuild.
+ auto OtherSourceContents = R"cpp(
+ #include "foo.h"
+ int c = d;
+ )cpp";
+ ASSERT_TRUE(DoUpdate(getInputs(Source, OtherSourceContents)));
+ ASSERT_FALSE(DoUpdate(getInputs(Source, OtherSourceContents)));
+
+ // Update to the compile commands should also cause a rebuild.
+ CDB.ExtraClangFlags.push_back("-DSOMETHING");
+ ASSERT_TRUE(DoUpdate(getInputs(Source, OtherSourceContents)));
+ ASSERT_FALSE(DoUpdate(getInputs(Source, OtherSourceContents)));
+}
+
} // namespace clangd
} // namespace clang
Modified: clang-tools-extra/trunk/unittests/clangd/TestFS.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TestFS.cpp?rev=338012&r1=338011&r2=338012&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/TestFS.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TestFS.cpp Thu Jul 26 02:21:07 2018
@@ -19,13 +19,15 @@ namespace clangd {
using namespace llvm;
IntrusiveRefCntPtr<vfs::FileSystem>
-buildTestFS(StringMap<std::string> const &Files) {
+buildTestFS(llvm::StringMap<std::string> const &Files,
+ llvm::StringMap<time_t> const &Timestamps) {
IntrusiveRefCntPtr<vfs::InMemoryFileSystem> MemFS(
new vfs::InMemoryFileSystem);
for (auto &FileAndContents : Files) {
- MemFS->addFile(FileAndContents.first(), time_t(),
- MemoryBuffer::getMemBufferCopy(FileAndContents.second,
- FileAndContents.first()));
+ StringRef File = FileAndContents.first();
+ MemFS->addFile(
+ File, Timestamps.lookup(File),
+ MemoryBuffer::getMemBufferCopy(FileAndContents.second, File));
}
return MemFS;
}
Modified: clang-tools-extra/trunk/unittests/clangd/TestFS.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TestFS.h?rev=338012&r1=338011&r2=338012&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/TestFS.h (original)
+++ clang-tools-extra/trunk/unittests/clangd/TestFS.h Thu Jul 26 02:21:07 2018
@@ -23,7 +23,8 @@ namespace clangd {
// Builds a VFS that provides access to the provided files, plus temporary
// directories.
llvm::IntrusiveRefCntPtr<vfs::FileSystem>
-buildTestFS(llvm::StringMap<std::string> const &Files);
+buildTestFS(llvm::StringMap<std::string> const &Files,
+ llvm::StringMap<time_t> const &Timestamps = {});
// A VFS provider that returns TestFSes containing a provided set of files.
class MockFSProvider : public FileSystemProvider {
More information about the cfe-commits
mailing list