[PATCH] D48940: [clangd] Wait for first preamble before code completion
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE336538: [clangd] Wait for first preamble before code
completion (authored by ibiryukov, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D48940?vs=154562=154563#toc
Repository:
rL LLVM
https://reviews.llvm.org/D48940
Files:
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -19,6 +19,7 @@
namespace clangd {
using ::testing::_;
+using ::testing::Each;
using ::testing::AnyOf;
using ::testing::Pair;
using ::testing::Pointee;
@@ -299,5 +300,40 @@
UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
}
+TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
+ // Testing strategy: we update the file and schedule a few preamble reads at
+ // the same time. All reads should get the same non-null preamble.
+ TUScheduler S(
+ /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+ PreambleParsedCallback(),
+ /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ ASTRetentionPolicy());
+ auto Foo = testPath("foo.cpp");
+ auto NonEmptyPreamble = R"cpp(
+#define FOO 1
+#define BAR 2
+
+int main() {}
+ )cpp";
+ constexpr int ReadsToSchedule = 10;
+ std::mutex PreamblesMut;
+ std::vector Preambles(ReadsToSchedule, nullptr);
+ S.update(Foo, getInputs(Foo, NonEmptyPreamble), WantDiagnostics::Auto,
+ [](std::vector) {});
+ for (int I = 0; I < ReadsToSchedule; ++I) {
+S.runWithPreamble(
+"test", Foo,
+[I, , ](llvm::Expected IP) {
+ std::lock_guard Lock(PreamblesMut);
+ Preambles[I] = cantFail(std::move(IP)).Preamble;
+});
+ }
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ // Check all actions got the same non-null preamble.
+ std::lock_guard Lock(PreamblesMut);
+ ASSERT_NE(Preambles[0], nullptr);
+ ASSERT_THAT(Preambles, Each(Preambles[0]));
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -101,6 +101,9 @@
/// - validate that the preamble is still valid, and only use it in this case
/// - accept that preamble contents may be outdated, and try to avoid reading
/// source code from headers.
+ /// If there's no preamble yet (because the file was just opened), we'll wait
+ /// for it to build. The preamble may still be null if it fails to build or is
+ /// empty.
/// If an error occurs during processing, it is forwarded to the \p Action
/// callback.
void runWithPreamble(llvm::StringRef Name, PathRef File,
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -176,6 +176,12 @@
bool blockUntilIdle(Deadline Timeout) const;
std::shared_ptr getPossiblyStalePreamble() const;
+ /// Wait for the first build of preamble to finish. Preamble itself can be
+ /// accessed via getPossibleStalePreamble(). Note that this function will
+ /// return after an unsuccessful build of the preamble too, i.e. result of
+ /// getPossiblyStalePreamble() can be null even after this function returns.
+ void waitForFirstPreamble() const;
+
std::size_t getUsedBytes() const;
bool isASTCached() const;
@@ -226,6 +232,8 @@
/// Guards members used by both TUScheduler and the worker thread.
mutable std::mutex Mutex;
std::shared_ptr LastBuiltPreamble; /* GUARDED_BY(Mutex) */
+ /// Becomes ready when the first preamble build finishes.
+ Notification PreambleWasBuilt;
/// Set to true to signal run() to finish processing.
bool Done;/* GUARDED_BY(Mutex) */
std::deque Requests; /* GUARDED_BY(Mutex) */
@@ -329,6 +337,9 @@
buildCompilerInvocation(Inputs);
if (!Invocation) {
log("Could not build CompilerInvocation for file " + FileName);
+ // Make sure anyone waiting for the preamble gets notified it could not
+ // be built.
+ PreambleWasBuilt.notify();
return;
}
@@ -340,6 +351,8 @@
if (NewPreamble)
LastBuiltPreamble = NewPreamble;
}
+PreambleWasBuilt.notify();
+
// Build the AST for diagnostics.
llvm::Optional AST =
buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs);
@@ -392,6 +405,10 @@
return LastBuiltPreamble;
}
+void ASTWorker::waitForFirstPreamble() const {
+ PreambleWasBuilt.wait();
+}
+
std::size_t ASTWorker::getUsedBytes() const {
// Note that we don't report the size of ASTs currently used for processing
// the in-flight requests. We used this information for debugging purposes
@@ -655,6 +672,11 @@
[PATCH] D48940: [clangd] Wait for first preamble before code completion
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336538: [clangd] Wait for first preamble before code
completion (authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D48940
Files:
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
@@ -19,6 +19,7 @@
namespace clangd {
using ::testing::_;
+using ::testing::Each;
using ::testing::AnyOf;
using ::testing::Pair;
using ::testing::Pointee;
@@ -299,5 +300,40 @@
UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
}
+TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
+ // Testing strategy: we update the file and schedule a few preamble reads at
+ // the same time. All reads should get the same non-null preamble.
+ TUScheduler S(
+ /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+ PreambleParsedCallback(),
+ /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ ASTRetentionPolicy());
+ auto Foo = testPath("foo.cpp");
+ auto NonEmptyPreamble = R"cpp(
+#define FOO 1
+#define BAR 2
+
+int main() {}
+ )cpp";
+ constexpr int ReadsToSchedule = 10;
+ std::mutex PreamblesMut;
+ std::vector Preambles(ReadsToSchedule, nullptr);
+ S.update(Foo, getInputs(Foo, NonEmptyPreamble), WantDiagnostics::Auto,
+ [](std::vector) {});
+ for (int I = 0; I < ReadsToSchedule; ++I) {
+S.runWithPreamble(
+"test", Foo,
+[I, , ](llvm::Expected IP) {
+ std::lock_guard Lock(PreamblesMut);
+ Preambles[I] = cantFail(std::move(IP)).Preamble;
+});
+ }
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ // Check all actions got the same non-null preamble.
+ std::lock_guard Lock(PreamblesMut);
+ ASSERT_NE(Preambles[0], nullptr);
+ ASSERT_THAT(Preambles, Each(Preambles[0]));
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/trunk/clangd/TUScheduler.h
===
--- clang-tools-extra/trunk/clangd/TUScheduler.h
+++ clang-tools-extra/trunk/clangd/TUScheduler.h
@@ -101,6 +101,9 @@
/// - validate that the preamble is still valid, and only use it in this case
/// - accept that preamble contents may be outdated, and try to avoid reading
/// source code from headers.
+ /// If there's no preamble yet (because the file was just opened), we'll wait
+ /// for it to build. The preamble may still be null if it fails to build or is
+ /// empty.
/// If an error occurs during processing, it is forwarded to the \p Action
/// callback.
void runWithPreamble(llvm::StringRef Name, PathRef File,
Index: clang-tools-extra/trunk/clangd/TUScheduler.cpp
===
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp
@@ -176,6 +176,12 @@
bool blockUntilIdle(Deadline Timeout) const;
std::shared_ptr getPossiblyStalePreamble() const;
+ /// Wait for the first build of preamble to finish. Preamble itself can be
+ /// accessed via getPossibleStalePreamble(). Note that this function will
+ /// return after an unsuccessful build of the preamble too, i.e. result of
+ /// getPossiblyStalePreamble() can be null even after this function returns.
+ void waitForFirstPreamble() const;
+
std::size_t getUsedBytes() const;
bool isASTCached() const;
@@ -226,6 +232,8 @@
/// Guards members used by both TUScheduler and the worker thread.
mutable std::mutex Mutex;
std::shared_ptr LastBuiltPreamble; /* GUARDED_BY(Mutex) */
+ /// Becomes ready when the first preamble build finishes.
+ Notification PreambleWasBuilt;
/// Set to true to signal run() to finish processing.
bool Done;/* GUARDED_BY(Mutex) */
std::deque Requests; /* GUARDED_BY(Mutex) */
@@ -329,6 +337,9 @@
buildCompilerInvocation(Inputs);
if (!Invocation) {
log("Could not build CompilerInvocation for file " + FileName);
+ // Make sure anyone waiting for the preamble gets notified it could not
+ // be built.
+ PreambleWasBuilt.notify();
return;
}
@@ -340,6 +351,8 @@
if (NewPreamble)
LastBuiltPreamble = NewPreamble;
}
+PreambleWasBuilt.notify();
+
// Build the AST for diagnostics.
llvm::Optional AST =
buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs);
@@ -392,6 +405,10 @@
return LastBuiltPreamble;
}
+void ASTWorker::waitForFirstPreamble() const {
+ PreambleWasBuilt.wait();
+}
+
[PATCH] D48940: [clangd] Wait for first preamble before code completion
ilya-biryukov updated this revision to Diff 154562.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
- Add a test
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48940
Files:
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -19,6 +19,7 @@
namespace clangd {
using ::testing::_;
+using ::testing::Each;
using ::testing::AnyOf;
using ::testing::Pair;
using ::testing::Pointee;
@@ -299,5 +300,40 @@
UnorderedElementsAre(Foo, AnyOf(Bar, Baz)));
}
+TEST_F(TUSchedulerTests, RunWaitsForPreamble) {
+ // Testing strategy: we update the file and schedule a few preamble reads at
+ // the same time. All reads should get the same non-null preamble.
+ TUScheduler S(
+ /*AsyncThreadsCount=*/4, /*StorePreambleInMemory=*/true,
+ PreambleParsedCallback(),
+ /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ ASTRetentionPolicy());
+ auto Foo = testPath("foo.cpp");
+ auto NonEmptyPreamble = R"cpp(
+#define FOO 1
+#define BAR 2
+
+int main() {}
+ )cpp";
+ constexpr int ReadsToSchedule = 10;
+ std::mutex PreamblesMut;
+ std::vector Preambles(ReadsToSchedule, nullptr);
+ S.update(Foo, getInputs(Foo, NonEmptyPreamble), WantDiagnostics::Auto,
+ [](std::vector) {});
+ for (int I = 0; I < ReadsToSchedule; ++I) {
+S.runWithPreamble(
+"test", Foo,
+[I, , ](llvm::Expected IP) {
+ std::lock_guard Lock(PreamblesMut);
+ Preambles[I] = cantFail(std::move(IP)).Preamble;
+});
+ }
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ // Check all actions got the same non-null preamble.
+ std::lock_guard Lock(PreamblesMut);
+ ASSERT_NE(Preambles[0], nullptr);
+ ASSERT_THAT(Preambles, Each(Preambles[0]));
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -101,6 +101,9 @@
/// - validate that the preamble is still valid, and only use it in this case
/// - accept that preamble contents may be outdated, and try to avoid reading
/// source code from headers.
+ /// If there's no preamble yet (because the file was just opened), we'll wait
+ /// for it to build. The preamble may still be null if it fails to build or is
+ /// empty.
/// If an error occurs during processing, it is forwarded to the \p Action
/// callback.
void runWithPreamble(llvm::StringRef Name, PathRef File,
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -176,6 +176,12 @@
bool blockUntilIdle(Deadline Timeout) const;
std::shared_ptr getPossiblyStalePreamble() const;
+ /// Wait for the first build of preamble to finish. Preamble itself can be
+ /// accessed via getPossibleStalePreamble(). Note that this function will
+ /// return after an unsuccessful build of the preamble too, i.e. result of
+ /// getPossiblyStalePreamble() can be null even after this function returns.
+ void waitForFirstPreamble() const;
+
std::size_t getUsedBytes() const;
bool isASTCached() const;
@@ -226,6 +232,8 @@
/// Guards members used by both TUScheduler and the worker thread.
mutable std::mutex Mutex;
std::shared_ptr LastBuiltPreamble; /* GUARDED_BY(Mutex) */
+ /// Becomes ready when the first preamble build finishes.
+ Notification PreambleWasBuilt;
/// Set to true to signal run() to finish processing.
bool Done;/* GUARDED_BY(Mutex) */
std::deque Requests; /* GUARDED_BY(Mutex) */
@@ -329,6 +337,9 @@
buildCompilerInvocation(Inputs);
if (!Invocation) {
log("Could not build CompilerInvocation for file " + FileName);
+ // Make sure anyone waiting for the preamble gets notified it could not
+ // be built.
+ PreambleWasBuilt.notify();
return;
}
@@ -340,6 +351,8 @@
if (NewPreamble)
LastBuiltPreamble = NewPreamble;
}
+PreambleWasBuilt.notify();
+
// Build the AST for diagnostics.
llvm::Optional AST =
buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs);
@@ -392,6 +405,10 @@
return LastBuiltPreamble;
}
+void ASTWorker::waitForFirstPreamble() const {
+ PreambleWasBuilt.wait();
+}
+
std::size_t ASTWorker::getUsedBytes() const {
// Note that we don't report the size of ASTs currently used for processing
// the in-flight requests. We used this information for debugging purposes
@@ -655,6 +672,11 @@
std::string Contents,
tooling::CompileCommand Command, Context Ctx,
[PATCH] D48940: [clangd] Wait for first preamble before code completion
ilya-biryukov added a comment.
In https://reviews.llvm.org/D48940#1152750, @sammccall wrote:
> Possible test: add a file with complicated preamble (billion laughs?) and
> immediately schedule 5 preamble actions. They should all get a non-null
> preamble and the pointers should all be the same.
The most trivial test that updates a file and immediately schedules a few
preamble actions failed without this change for me. I have added it to the
patch.
Comment at: clangd/TUScheduler.cpp:408
+void ASTWorker::waitForFirstPreamble() const {
+ PreambleWasBuilt.wait();
sammccall wrote:
> inline? should fit on one line...
This is used in TUScheduler, where we don't have access to the fields of
ASTWorker.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48940
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48940: [clangd] Wait for first preamble before code completion
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Nice fix!
Possible test: add a file with complicated preamble (billion laughs?) and
immediately schedule 5 preamble actions. They should all get a non-null
preamble and the pointers should all be the same.
Comment at: clangd/TUScheduler.cpp:408
+void ASTWorker::waitForFirstPreamble() const {
+ PreambleWasBuilt.wait();
inline? should fit on one line...
Comment at: clangd/TUScheduler.h:104
/// source code from headers.
+ /// However, Action will be scheduled to run after the first rebuild of the
+ /// preamble for the corresponding file finishes. Note that the rebuild can
Not sure what "however" is contrasting with.
Could be just: "If there's no preamble yet (because the file was just opened),
we'll wait for it to build. The preamble may still be null if it fails to build
or is empty."
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48940
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48940: [clangd] Wait for first preamble before code completion
ilya-biryukov created this revision.
ilya-biryukov added a reviewer: sammccall.
Herald added subscribers: jkorous, MaskRay, ioeric, javed.absar.
To avoid doing extra work of processing headers in the preamble
mutilple times in parallel.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48940
Files:
clangd/TUScheduler.cpp
clangd/TUScheduler.h
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -101,6 +101,10 @@
/// - validate that the preamble is still valid, and only use it in this case
/// - accept that preamble contents may be outdated, and try to avoid reading
/// source code from headers.
+ /// However, Action will be scheduled to run after the first rebuild of the
+ /// preamble for the corresponding file finishes. Note that the rebuild can
+ /// still fail, in which case Action will get a null pointer instead of the
+ /// preamble.
/// If an error occurs during processing, it is forwarded to the \p Action
/// callback.
void runWithPreamble(llvm::StringRef Name, PathRef File,
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -176,6 +176,12 @@
bool blockUntilIdle(Deadline Timeout) const;
std::shared_ptr getPossiblyStalePreamble() const;
+ /// Wait for the first build of preamble to finish. Preamble itself can be
+ /// accessed via getPossibleStalePreamble(). Note that this function will
+ /// return after an unsuccessful build of the preamble too, i.e. result of
+ /// getPossiblyStalePreamble() can be null even after this function returns.
+ void waitForFirstPreamble() const;
+
std::size_t getUsedBytes() const;
bool isASTCached() const;
@@ -226,6 +232,8 @@
/// Guards members used by both TUScheduler and the worker thread.
mutable std::mutex Mutex;
std::shared_ptr LastBuiltPreamble; /* GUARDED_BY(Mutex)
*/
+ /// Becomes ready when the first preamble build finishes.
+ Notification PreambleWasBuilt;
/// Set to true to signal run() to finish processing.
bool Done;/* GUARDED_BY(Mutex) */
std::deque Requests; /* GUARDED_BY(Mutex) */
@@ -329,6 +337,9 @@
buildCompilerInvocation(Inputs);
if (!Invocation) {
log("Could not build CompilerInvocation for file " + FileName);
+ // Make sure anyone waiting for the preamble gets notified it could not
+ // be built.
+ PreambleWasBuilt.notify();
return;
}
@@ -340,6 +351,8 @@
if (NewPreamble)
LastBuiltPreamble = NewPreamble;
}
+PreambleWasBuilt.notify();
+
// Build the AST for diagnostics.
llvm::Optional AST =
buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs);
@@ -392,6 +405,10 @@
return LastBuiltPreamble;
}
+void ASTWorker::waitForFirstPreamble() const {
+ PreambleWasBuilt.wait();
+}
+
std::size_t ASTWorker::getUsedBytes() const {
// Note that we don't report the size of ASTs currently used for processing
// the in-flight requests. We used this information for debugging purposes
@@ -655,6 +672,11 @@
std::string Contents,
tooling::CompileCommand Command, Context Ctx,
decltype(Action) Action) mutable {
+// We don't want to be running preamble actions before the preamble was
+// built for the first time. This avoids extra work of processing the
+// preamble headers in parallel multiple times.
+Worker->waitForFirstPreamble();
+
std::lock_guard BarrierLock(Barrier);
WithContext Guard(std::move(Ctx));
trace::Span Tracer(Name);
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -101,6 +101,10 @@
/// - validate that the preamble is still valid, and only use it in this case
/// - accept that preamble contents may be outdated, and try to avoid reading
/// source code from headers.
+ /// However, Action will be scheduled to run after the first rebuild of the
+ /// preamble for the corresponding file finishes. Note that the rebuild can
+ /// still fail, in which case Action will get a null pointer instead of the
+ /// preamble.
/// If an error occurs during processing, it is forwarded to the \p Action
/// callback.
void runWithPreamble(llvm::StringRef Name, PathRef File,
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -176,6 +176,12 @@
bool blockUntilIdle(Deadline Timeout) const;
std::shared_ptr getPossiblyStalePreamble() const;
+ /// Wait for the first build of preamble to finish. Preamble itself can be
+ /// accessed via getPossibleStalePreamble(). Note that this function will
+ /// return
