ilya-biryukov updated this revision to Diff 157434.
ilya-biryukov added a comment.
- Move OldPreamble.reset() out of the lock, add a comment
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49783
Files:
clangd/TUScheduler.cpp
test/clangd/extra-flags.test
unittests/clangd/TUSchedulerTests.cpp
unittests/clangd/TestFS.cpp
unittests/clangd/TestFS.h
Index: unittests/clangd/TestFS.h
===================================================================
--- unittests/clangd/TestFS.h
+++ unittests/clangd/TestFS.h
@@ -23,7 +23,8 @@
// 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 {
Index: unittests/clangd/TestFS.cpp
===================================================================
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -19,13 +19,15 @@
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;
}
Index: unittests/clangd/TUSchedulerTests.cpp
===================================================================
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -33,13 +33,12 @@
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 @@
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 @@
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 @@
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
Index: test/clangd/extra-flags.test
===================================================================
--- test/clangd/extra-flags.test
+++ test/clangd/extra-flags.test
@@ -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": [
Index: clangd/TUScheduler.cpp
===================================================================
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -324,6 +324,11 @@
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 @@
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);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
