[clang-tools-extra] ed98994 - Revert "[clangd] Mechanism to make update debounce responsive to rebuild speed."
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 4 06:34:23 PST 2020
Author: Sam McCall
Date: 2020-02-04T15:34:10+01:00
New Revision: ed98994f64b8fe6443aef57a5faa953e86d9fc0e
URL: https://github.com/llvm/llvm-project/commit/ed98994f64b8fe6443aef57a5faa953e86d9fc0e
DIFF: https://github.com/llvm/llvm-project/commit/ed98994f64b8fe6443aef57a5faa953e86d9fc0e.diff
LOG: Revert "[clangd] Mechanism to make update debounce responsive to rebuild speed."
This reverts commit 92570718a86cc4c23108b596002114ab25857b14.
Breaking tests: http://45.33.8.238/linux/9296/step_9.txt
Added:
Modified:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 3e22a4dfe667..00da7af06741 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -106,7 +106,7 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
ClangdServer::Options ClangdServer::optsForTest() {
ClangdServer::Options Opts;
- Opts.UpdateDebounce = DebouncePolicy::fixed(/*zero*/ {});
+ Opts.UpdateDebounce = std::chrono::steady_clock::duration::zero(); // Faster!
Opts.StorePreamblesInMemory = true;
Opts.AsyncThreadsCount = 4; // Consistent!
Opts.SemanticHighlighting = true;
diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index 3c3505295a75..2d0957f441bb 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -130,8 +130,8 @@ class ClangdServer {
llvm::Optional<std::string> ResourceDir = llvm::None;
/// Time to wait after a new file version before computing diagnostics.
- DebouncePolicy UpdateDebounce =
- DebouncePolicy::fixed(std::chrono::milliseconds(500));
+ std::chrono::steady_clock::duration UpdateDebounce =
+ std::chrono::milliseconds(500);
bool SuggestMissingIncludes = false;
diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 237c4c4d7ba6..9218ae9a03ce 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -61,7 +61,6 @@
#include "llvm/Support/Threading.h"
#include <algorithm>
#include <memory>
-#include <mutex>
#include <queue>
#include <thread>
@@ -165,7 +164,7 @@ class ASTWorker {
friend class ASTWorkerHandle;
ASTWorker(PathRef FileName, const GlobalCompilationDatabase &CDB,
TUScheduler::ASTCache &LRUCache, Semaphore &Barrier, bool RunSync,
- DebouncePolicy UpdateDebounce, bool StorePreamblesInMemory,
+ steady_clock::duration UpdateDebounce, bool StorePreamblesInMemory,
ParsingCallbacks &Callbacks);
public:
@@ -177,7 +176,7 @@ class ASTWorker {
static ASTWorkerHandle
create(PathRef FileName, const GlobalCompilationDatabase &CDB,
TUScheduler::ASTCache &IdleASTs, AsyncTaskRunner *Tasks,
- Semaphore &Barrier, DebouncePolicy UpdateDebounce,
+ Semaphore &Barrier, steady_clock::duration UpdateDebounce,
bool StorePreamblesInMemory, ParsingCallbacks &Callbacks);
~ASTWorker();
@@ -243,7 +242,7 @@ class ASTWorker {
TUScheduler::ASTCache &IdleASTs;
const bool RunSync;
/// Time to wait after an update to see whether another update obsoletes it.
- const DebouncePolicy UpdateDebounce;
+ const steady_clock::duration UpdateDebounce;
/// File that ASTWorker is responsible for.
const Path FileName;
const GlobalCompilationDatabase &CDB;
@@ -264,9 +263,6 @@ class ASTWorker {
/// be consumed by clients of ASTWorker.
std::shared_ptr<const ParseInputs> FileInputs; /* GUARDED_BY(Mutex) */
std::shared_ptr<const PreambleData> LastBuiltPreamble; /* GUARDED_BY(Mutex) */
- /// Times of recent AST rebuilds, used for UpdateDebounce computation.
- llvm::SmallVector<DebouncePolicy::clock::duration, 8>
- RebuildTimes; /* GUARDED_BY(Mutex) */
/// Becomes ready when the first preamble build finishes.
Notification PreambleWasBuilt;
/// Set to true to signal run() to finish processing.
@@ -330,7 +326,7 @@ class ASTWorkerHandle {
ASTWorkerHandle
ASTWorker::create(PathRef FileName, const GlobalCompilationDatabase &CDB,
TUScheduler::ASTCache &IdleASTs, AsyncTaskRunner *Tasks,
- Semaphore &Barrier, DebouncePolicy UpdateDebounce,
+ Semaphore &Barrier, steady_clock::duration UpdateDebounce,
bool StorePreamblesInMemory, ParsingCallbacks &Callbacks) {
std::shared_ptr<ASTWorker> Worker(
new ASTWorker(FileName, CDB, IdleASTs, Barrier, /*RunSync=*/!Tasks,
@@ -344,7 +340,7 @@ ASTWorker::create(PathRef FileName, const GlobalCompilationDatabase &CDB,
ASTWorker::ASTWorker(PathRef FileName, const GlobalCompilationDatabase &CDB,
TUScheduler::ASTCache &LRUCache, Semaphore &Barrier,
- bool RunSync, DebouncePolicy UpdateDebounce,
+ bool RunSync, steady_clock::duration UpdateDebounce,
bool StorePreamblesInMemory, ParsingCallbacks &Callbacks)
: IdleASTs(LRUCache), RunSync(RunSync), UpdateDebounce(UpdateDebounce),
FileName(FileName), CDB(CDB),
@@ -492,7 +488,6 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
// Get the AST for diagnostics.
llvm::Optional<std::unique_ptr<ParsedAST>> AST = IdleASTs.take(this);
- auto RebuildStartTime = DebouncePolicy::clock::now();
if (!AST) {
llvm::Optional<ParsedAST> NewAST =
buildAST(FileName, std::move(Invocation), CompilerInvocationDiags,
@@ -515,19 +510,6 @@ void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
// spam us with updates.
// Note *AST can still be null if buildAST fails.
if (*AST) {
- {
- // Try to record the AST-build time, to inform future update debouncing.
- // This is best-effort only: if the lock is held, don't bother.
- auto RebuildDuration = DebouncePolicy::clock::now() - RebuildStartTime;
- std::unique_lock<std::mutex> Lock(Mutex, std::try_to_lock);
- if (Lock.owns_lock()) {
- // Do not let RebuildTimes grow beyond its small-size (i.e. capacity).
- if (RebuildTimes.size() == RebuildTimes.capacity())
- RebuildTimes.erase(RebuildTimes.begin());
- RebuildTimes.push_back(RebuildDuration);
- Mutex.unlock();
- }
- }
trace::Span Span("Running main AST callback");
Callbacks.onMainAST(FileName, **AST, RunPublish);
@@ -768,13 +750,13 @@ Deadline ASTWorker::scheduleLocked() {
assert(!Requests.empty() && "skipped the whole queue");
// Some updates aren't dead yet, but never end up being used.
// e.g. the first keystroke is live until obsoleted by the second.
- // We debounce "maybe-unused" writes, sleeping in case they become dead.
+ // We debounce "maybe-unused" writes, sleeping 500ms in case they become dead.
// But don't delay reads (including updates where diagnostics are needed).
for (const auto &R : Requests)
if (R.UpdateType == None || R.UpdateType == WantDiagnostics::Yes)
return Deadline::zero();
// Front request needs to be debounced, so determine when we're ready.
- Deadline D(Requests.front().AddTime + UpdateDebounce.compute(RebuildTimes));
+ Deadline D(Requests.front().AddTime + UpdateDebounce);
return D;
}
@@ -1054,33 +1036,5 @@ std::vector<Path> TUScheduler::getFilesWithCachedAST() const {
return Result;
}
-DebouncePolicy::clock::duration
-DebouncePolicy::compute(llvm::ArrayRef<clock::duration> History) const {
- assert(Min <= Max && "Invalid policy");
- if (History.empty())
- return Max; // Arbitrary.
-
- // Base the result on the median rebuild.
- // nth_element needs a mutable array, take the chance to bound the data size.
- History = History.take_back(15);
- llvm::SmallVector<clock::duration, 15> Recent(History.begin(), History.end());
- auto Median = Recent.begin() + Recent.size() / 2;
- std::nth_element(Recent.begin(), Median, Recent.end());
-
- clock::duration Target =
- std::chrono::duration_cast<clock::duration>(RebuildRatio * *Median);
- if (Target > Max)
- return Max;
- if (Target < Min)
- return Min;
- return Target;
-}
-
-DebouncePolicy DebouncePolicy::fixed(clock::duration T) {
- DebouncePolicy P;
- P.Min = P.Max = T;
- return P;
-}
-
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h
index 5082612b0ccc..e59bebaea330 100644
--- a/clang-tools-extra/clangd/TUScheduler.h
+++ b/clang-tools-extra/clangd/TUScheduler.h
@@ -61,28 +61,6 @@ struct ASTRetentionPolicy {
unsigned MaxRetainedASTs = 3;
};
-/// Clangd may wait after an update to see if another one comes along.
-/// This is so we rebuild once the user stops typing, not when they start.
-/// Debounce may be disabled/interrupted if we must build this version.
-/// The debounce time is responsive to user preferences and rebuild time.
-/// In the future, we could also consider
diff erent types of edits.
-struct DebouncePolicy {
- using clock = std::chrono::steady_clock;
-
- /// The minimum time that we always debounce for.
- clock::duration Min = /*zero*/ {};
- /// The maximum time we may debounce for.
- clock::duration Max = /*zero*/ {};
- /// Target debounce, as a fraction of file rebuild time.
- /// e.g. RebuildRatio = 2, recent builds took 200ms => debounce for 400ms.
- float RebuildRatio = 1;
-
- /// Compute the time to debounce based on this policy and recent build times.
- clock::duration compute(llvm::ArrayRef<clock::duration> History) const;
- /// A policy that always returns the same duration, useful for tests.
- static DebouncePolicy fixed(clock::duration);
-};
-
struct TUAction {
enum State {
Queued, // The TU is pending in the thread task queue to be built.
@@ -180,7 +158,7 @@ class TUScheduler {
/// Time to wait after an update to see if another one comes along.
/// This tries to ensure we rebuild once the user stops typing.
- DebouncePolicy UpdateDebounce;
+ std::chrono::steady_clock::duration UpdateDebounce = /*zero*/ {};
/// Determines when to keep idle ASTs in memory for future use.
ASTRetentionPolicy RetentionPolicy;
@@ -295,7 +273,7 @@ class TUScheduler {
// asynchronously.
llvm::Optional<AsyncTaskRunner> PreambleTasks;
llvm::Optional<AsyncTaskRunner> WorkerThreads;
- DebouncePolicy UpdateDebounce;
+ std::chrono::steady_clock::duration UpdateDebounce;
};
} // namespace clangd
diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
index 6605ccf65860..053d4aaefaa6 100644
--- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -23,7 +23,6 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include <algorithm>
-#include <chrono>
#include <utility>
namespace clang {
@@ -209,7 +208,7 @@ TEST_F(TUSchedulerTests, Debounce) {
std::atomic<int> CallbackCount(0);
{
auto Opts = optsForTest();
- Opts.UpdateDebounce = DebouncePolicy::fixed(std::chrono::seconds(1));
+ Opts.UpdateDebounce = std::chrono::seconds(1);
TUScheduler S(CDB, Opts, captureDiags());
// FIXME: we could probably use timeouts lower than 1 second here.
auto Path = testPath("foo.cpp");
@@ -362,7 +361,7 @@ TEST_F(TUSchedulerTests, ManyUpdates) {
// Run TUScheduler and collect some stats.
{
auto Opts = optsForTest();
- Opts.UpdateDebounce = DebouncePolicy::fixed(std::chrono::milliseconds(50));
+ Opts.UpdateDebounce = std::chrono::milliseconds(50);
TUScheduler S(CDB, Opts, captureDiags());
std::vector<std::string> Files;
@@ -755,32 +754,6 @@ TEST_F(TUSchedulerTests, CommandLineWarnings) {
EXPECT_THAT(Diagnostics, IsEmpty());
}
-TEST(DebouncePolicy, Compute) {
- namespace c = std::chrono;
- std::vector<DebouncePolicy::clock::duration> History = {
- c::seconds(0),
- c::seconds(5),
- c::seconds(10),
- c::seconds(20),
- };
- DebouncePolicy Policy;
- Policy.Min = c::seconds(3);
- Policy.Max = c::seconds(25);
- // Call Policy.compute(History) and return seconds as a float.
- auto Compute = [&](llvm::ArrayRef<DebouncePolicy::clock::duration> History) {
- using FloatingSeconds = c::duration<float, c::seconds::period>;
- return static_cast<float>(Policy.compute(History) / FloatingSeconds(1));
- };
- EXPECT_NEAR(10, Compute(History), 0.01) << "(upper) median = 10";
- Policy.RebuildRatio = 1.5;
- EXPECT_NEAR(15, Compute(History), 0.01) << "median = 10, ratio = 1.5";
- Policy.RebuildRatio = 3;
- EXPECT_NEAR(25, Compute(History), 0.01) << "constrained by max";
- Policy.RebuildRatio = 0;
- EXPECT_NEAR(3, Compute(History), 0.01) << "constrained by min";
- EXPECT_NEAR(25, Compute({}), 0.01) << "no history -> max";
-}
-
} // namespace
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list