The TUSchedulerTest::Debounce breaks on Windows buildbots. Probably because the timeouts of 50ms/10ms/40ms are too low to be scheduled properly. Increased the timeouts in r326598 to 1s/200ms/2s, hopefully that would unbreak the buildbots.
We could tweak them back to lower values that work on Monday :-) On Fri, Mar 2, 2018 at 9:58 AM Sam McCall via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: sammccall > Date: Fri Mar 2 00:56:37 2018 > New Revision: 326546 > > URL: http://llvm.org/viewvc/llvm-project?rev=326546&view=rev > Log: > [clangd] Debounce streams of updates. > > Summary: > Don't actually start building ASTs for new revisions until either: > - 500ms have passed since the last revision, or > - we actually need the revision for something (or to unblock the queue) > > In practice, this avoids the "first keystroke results in diagnostics" > problem. > This is kind of awkward to test, and the test is pretty bad. > It can be observed nicely by capturing a trace, though. > > Reviewers: hokein, ilya-biryukov > > Subscribers: klimek, jkorous-apple, ioeric, cfe-commits > > Differential Revision: https://reviews.llvm.org/D43648 > > Modified: > clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp > clang-tools-extra/trunk/clangd/ClangdServer.cpp > clang-tools-extra/trunk/clangd/ClangdServer.h > clang-tools-extra/trunk/clangd/TUScheduler.cpp > clang-tools-extra/trunk/clangd/TUScheduler.h > clang-tools-extra/trunk/clangd/Threading.cpp > clang-tools-extra/trunk/clangd/Threading.h > clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp > > Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=326546&r1=326545&r2=326546&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original) > +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Fri Mar 2 00:56:37 > 2018 > @@ -405,7 +405,7 @@ ClangdLSPServer::ClangdLSPServer(JSONOut > : Out(Out), CDB(std::move(CompileCommandsDir)), CCOpts(CCOpts), > Server(CDB, /*DiagConsumer=*/*this, FSProvider, AsyncThreadsCount, > StorePreamblesInMemory, BuildDynamicSymbolIndex, StaticIdx, > - ResourceDir) {} > + ResourceDir, > /*UpdateDebounce=*/std::chrono::milliseconds(500)) {} > > bool ClangdLSPServer::run(std::istream &In, JSONStreamStyle InputStyle) { > assert(!IsDone && "Run was called before"); > > Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=326546&r1=326545&r2=326546&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original) > +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Fri Mar 2 00:56:37 > 2018 > @@ -76,7 +76,8 @@ ClangdServer::ClangdServer(GlobalCompila > unsigned AsyncThreadsCount, > bool StorePreamblesInMemory, > bool BuildDynamicSymbolIndex, SymbolIndex > *StaticIdx, > - llvm::Optional<StringRef> ResourceDir) > + llvm::Optional<StringRef> ResourceDir, > + std::chrono::steady_clock::duration > UpdateDebounce) > : CompileArgs(CDB, > ResourceDir ? ResourceDir->str() : > getStandardResourceDir()), > DiagConsumer(DiagConsumer), FSProvider(FSProvider), > @@ -91,7 +92,8 @@ ClangdServer::ClangdServer(GlobalCompila > FileIdx > ? [this](PathRef Path, > ParsedAST *AST) { FileIdx->update(Path, > AST); } > - : ASTParsedCallback()) { > + : ASTParsedCallback(), > + UpdateDebounce) { > if (FileIdx && StaticIdx) { > MergedIndex = mergeIndex(FileIdx.get(), StaticIdx); > Index = MergedIndex.get(); > > Modified: clang-tools-extra/trunk/clangd/ClangdServer.h > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=326546&r1=326545&r2=326546&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/clangd/ClangdServer.h (original) > +++ clang-tools-extra/trunk/clangd/ClangdServer.h Fri Mar 2 00:56:37 2018 > @@ -125,6 +125,8 @@ public: > /// \p DiagConsumer. Note that a callback to \p DiagConsumer happens on > a > /// worker thread. Therefore, instances of \p DiagConsumer must properly > /// synchronize access to shared state. > + /// UpdateDebounce determines how long to wait after a new version of > the file > + /// before starting to compute diagnostics. > /// > /// \p StorePreamblesInMemory defines whether the Preambles generated by > /// clangd are stored in-memory or on disk. > @@ -135,13 +137,17 @@ public: > /// > /// If \p StaticIdx is set, ClangdServer uses the index for global code > /// completion. > + /// FIXME(sammccall): pull out an options struct. > ClangdServer(GlobalCompilationDatabase &CDB, > DiagnosticsConsumer &DiagConsumer, > FileSystemProvider &FSProvider, unsigned AsyncThreadsCount, > bool StorePreamblesInMemory, > bool BuildDynamicSymbolIndex = false, > SymbolIndex *StaticIdx = nullptr, > - llvm::Optional<StringRef> ResourceDir = llvm::None); > + llvm::Optional<StringRef> ResourceDir = llvm::None, > + /* Tiny default debounce, so tests hit the debounce logic > */ > + std::chrono::steady_clock::duration UpdateDebounce = > + std::chrono::milliseconds(20)); > > /// Set the root path of the workspace. > void setRootPath(PathRef RootPath); > > Modified: clang-tools-extra/trunk/clangd/TUScheduler.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.cpp?rev=326546&r1=326545&r2=326546&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/clangd/TUScheduler.cpp (original) > +++ clang-tools-extra/trunk/clangd/TUScheduler.cpp Fri Mar 2 00:56:37 2018 > @@ -54,6 +54,7 @@ > > namespace clang { > namespace clangd { > +using std::chrono::steady_clock; > namespace { > class ASTWorkerHandle; > > @@ -69,8 +70,8 @@ class ASTWorkerHandle; > /// worker. > class ASTWorker { > friend class ASTWorkerHandle; > - ASTWorker(llvm::StringRef File, Semaphore &Barrier, CppFile AST, > - bool RunSync); > + ASTWorker(llvm::StringRef File, Semaphore &Barrier, CppFile AST, bool > RunSync, > + steady_clock::duration UpdateDebounce); > > public: > /// Create a new ASTWorker and return a handle to it. > @@ -79,7 +80,8 @@ public: > /// synchronously instead. \p Barrier is acquired when processing each > /// request, it is be used to limit the number of actively running > threads. > static ASTWorkerHandle Create(llvm::StringRef File, AsyncTaskRunner > *Tasks, > - Semaphore &Barrier, CppFile AST); > + Semaphore &Barrier, CppFile AST, > + steady_clock::duration UpdateDebounce); > ~ASTWorker(); > > void update(ParseInputs Inputs, WantDiagnostics, > @@ -101,18 +103,27 @@ private: > /// Adds a new task to the end of the request queue. > void startTask(llvm::StringRef Name, UniqueFunction<void()> Task, > llvm::Optional<WantDiagnostics> UpdateType); > + /// Determines the next action to perform. > + /// All actions that should never run are disarded. > + /// Returns a deadline for the next action. If it's expired, run now. > + /// scheduleLocked() is called again at the deadline, or if requests > arrive. > + Deadline scheduleLocked(); > /// Should the first task in the queue be skipped instead of run? > bool shouldSkipHeadLocked() const; > > struct Request { > UniqueFunction<void()> Action; > std::string Name; > + steady_clock::time_point AddTime; > Context Ctx; > llvm::Optional<WantDiagnostics> UpdateType; > }; > > - std::string File; > + const std::string File; > const bool RunSync; > + // Time to wait after an update to see whether another update obsoletes > it. > + const steady_clock::duration UpdateDebounce; > + > Semaphore &Barrier; > // AST and FileInputs are only accessed on the processing thread from > run(). > CppFile AST; > @@ -172,9 +183,10 @@ private: > }; > > ASTWorkerHandle ASTWorker::Create(llvm::StringRef File, AsyncTaskRunner > *Tasks, > - Semaphore &Barrier, CppFile AST) { > - std::shared_ptr<ASTWorker> Worker( > - new ASTWorker(File, Barrier, std::move(AST), /*RunSync=*/!Tasks)); > + Semaphore &Barrier, CppFile AST, > + steady_clock::duration UpdateDebounce) { > + std::shared_ptr<ASTWorker> Worker(new ASTWorker( > + File, Barrier, std::move(AST), /*RunSync=*/!Tasks, UpdateDebounce)); > if (Tasks) > Tasks->runAsync("worker:" + llvm::sys::path::filename(File), > [Worker]() { Worker->run(); }); > @@ -183,9 +195,9 @@ ASTWorkerHandle ASTWorker::Create(llvm:: > } > > ASTWorker::ASTWorker(llvm::StringRef File, Semaphore &Barrier, CppFile > AST, > - bool RunSync) > - : File(File), RunSync(RunSync), Barrier(Barrier), AST(std::move(AST)), > - Done(false) { > + bool RunSync, steady_clock::duration UpdateDebounce) > + : File(File), RunSync(RunSync), UpdateDebounce(UpdateDebounce), > + Barrier(Barrier), AST(std::move(AST)), Done(false) { > if (RunSync) > return; > } > @@ -275,8 +287,8 @@ void ASTWorker::startTask(llvm::StringRe > { > std::lock_guard<std::mutex> Lock(Mutex); > assert(!Done && "running a task after stop()"); > - Requests.push_back( > - {std::move(Task), Name, Context::current().clone(), UpdateType}); > + Requests.push_back({std::move(Task), Name, steady_clock::now(), > + Context::current().clone(), UpdateType}); > } > RequestsCV.notify_all(); > } > @@ -286,17 +298,31 @@ void ASTWorker::run() { > Request Req; > { > std::unique_lock<std::mutex> Lock(Mutex); > - RequestsCV.wait(Lock, [&]() { return Done || !Requests.empty(); }); > - if (Requests.empty()) { > - assert(Done); > - return; > - } > - // Even when Done is true, we finish processing all pending requests > - // before exiting the processing loop. > + for (auto Wait = scheduleLocked(); !Wait.expired(); > + Wait = scheduleLocked()) { > + if (Done) { > + if (Requests.empty()) > + return; > + else // Even though Done is set, finish pending requests. > + break; // However, skip delays to shutdown fast. > + } > + > + // Tracing: we have a next request, attribute this sleep to it. > + Optional<WithContext> Ctx; > + Optional<trace::Span> Tracer; > + if (!Requests.empty()) { > + Ctx.emplace(Requests.front().Ctx.clone()); > + Tracer.emplace("Debounce"); > + SPAN_ATTACH(*Tracer, "next_request", Requests.front().Name); > + if (!(Wait == Deadline::infinity())) > + SPAN_ATTACH(*Tracer, "sleep_ms", > + > std::chrono::duration_cast<std::chrono::milliseconds>( > + Wait.time() - steady_clock::now()) > + .count()); > + } > > - while (shouldSkipHeadLocked()) > - Requests.pop_front(); > - assert(!Requests.empty() && "skipped the whole queue"); > + wait(Lock, RequestsCV, Wait); > + } > Req = std::move(Requests.front()); > // Leave it on the queue for now, so waiters don't see an empty > queue. > } // unlock Mutex > @@ -316,6 +342,24 @@ void ASTWorker::run() { > } > } > > +Deadline ASTWorker::scheduleLocked() { > + if (Requests.empty()) > + return Deadline::infinity(); // Wait for new requests. > + while (shouldSkipHeadLocked()) > + Requests.pop_front(); > + 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 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); > + return D; > +} > + > // Returns true if Requests.front() is a dead update that can be skipped. > bool ASTWorker::shouldSkipHeadLocked() const { > assert(!Requests.empty()); > @@ -370,10 +414,12 @@ struct TUScheduler::FileData { > > TUScheduler::TUScheduler(unsigned AsyncThreadsCount, > bool StorePreamblesInMemory, > - ASTParsedCallback ASTCallback) > + ASTParsedCallback ASTCallback, > + steady_clock::duration UpdateDebounce) > : StorePreamblesInMemory(StorePreamblesInMemory), > PCHOps(std::make_shared<PCHContainerOperations>()), > - ASTCallback(std::move(ASTCallback)), Barrier(AsyncThreadsCount) { > + ASTCallback(std::move(ASTCallback)), Barrier(AsyncThreadsCount), > + UpdateDebounce(UpdateDebounce) { > if (0 < AsyncThreadsCount) { > PreambleTasks.emplace(); > WorkerThreads.emplace(); > @@ -409,7 +455,8 @@ void TUScheduler::update( > // Create a new worker to process the AST-related tasks. > ASTWorkerHandle Worker = ASTWorker::Create( > File, WorkerThreads ? WorkerThreads.getPointer() : nullptr, > Barrier, > - CppFile(File, StorePreamblesInMemory, PCHOps, ASTCallback)); > + CppFile(File, StorePreamblesInMemory, PCHOps, ASTCallback), > + UpdateDebounce); > FD = std::unique_ptr<FileData>(new FileData{Inputs, > std::move(Worker)}); > } else { > FD->Inputs = Inputs; > > Modified: clang-tools-extra/trunk/clangd/TUScheduler.h > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.h?rev=326546&r1=326545&r2=326546&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/clangd/TUScheduler.h (original) > +++ clang-tools-extra/trunk/clangd/TUScheduler.h Fri Mar 2 00:56:37 2018 > @@ -17,6 +17,7 @@ > > namespace clang { > namespace clangd { > + > /// Returns a number of a default async threads to use for TUScheduler. > /// Returned value is always >= 1 (i.e. will not cause requests to be > processed > /// synchronously). > @@ -46,10 +47,12 @@ enum class WantDiagnostics { > /// and scheduling tasks. > /// Callbacks are run on a threadpool and it's appropriate to do slow > work in > /// them. Each task has a name, used for tracing (should be > UpperCamelCase). > +/// FIXME(sammccall): pull out a scheduler options struct. > class TUScheduler { > public: > TUScheduler(unsigned AsyncThreadsCount, bool StorePreamblesInMemory, > - ASTParsedCallback ASTCallback); > + ASTParsedCallback ASTCallback, > + std::chrono::steady_clock::duration UpdateDebounce); > ~TUScheduler(); > > /// Returns estimated memory usage for each of the currently open files. > @@ -101,6 +104,7 @@ private: > // asynchronously. > llvm::Optional<AsyncTaskRunner> PreambleTasks; > llvm::Optional<AsyncTaskRunner> WorkerThreads; > + std::chrono::steady_clock::duration UpdateDebounce; > }; > } // namespace clangd > } // namespace clang > > Modified: clang-tools-extra/trunk/clangd/Threading.cpp > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Threading.cpp?rev=326546&r1=326545&r2=326546&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/clangd/Threading.cpp (original) > +++ clang-tools-extra/trunk/clangd/Threading.cpp Fri Mar 2 00:56:37 2018 > @@ -76,10 +76,19 @@ void AsyncTaskRunner::runAsync(llvm::Twi > Deadline timeoutSeconds(llvm::Optional<double> Seconds) { > using namespace std::chrono; > if (!Seconds) > - return llvm::None; > + return Deadline::infinity(); > return steady_clock::now() + > > duration_cast<steady_clock::duration>(duration<double>(*Seconds)); > } > > +void wait(std::unique_lock<std::mutex> &Lock, std::condition_variable &CV, > + Deadline D) { > + if (D == Deadline::zero()) > + return; > + if (D == Deadline::infinity()) > + return CV.wait(Lock); > + CV.wait_until(Lock, D.time()); > +} > + > } // namespace clangd > } // namespace clang > > Modified: clang-tools-extra/trunk/clangd/Threading.h > URL: > http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Threading.h?rev=326546&r1=326545&r2=326546&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/clangd/Threading.h (original) > +++ clang-tools-extra/trunk/clangd/Threading.h Fri Mar 2 00:56:37 2018 > @@ -50,18 +50,50 @@ private: > std::size_t FreeSlots; > }; > > -/// A point in time we may wait for, or None to wait forever. > +/// A point in time we can wait for. > +/// Can be zero (don't wait) or infinity (wait forever). > /// (Not time_point::max(), because many std::chrono implementations > overflow). > -using Deadline = llvm::Optional<std::chrono::steady_clock::time_point>; > -/// Makes a deadline from a timeout in seconds. > +class Deadline { > +public: > + Deadline(std::chrono::steady_clock::time_point Time) > + : Type(Finite), Time(Time) {} > + static Deadline zero() { return Deadline(Zero); } > + static Deadline infinity() { return Deadline(Infinite); } > + > + std::chrono::steady_clock::time_point time() const { > + assert(Type == Finite); > + return Time; > + } > + bool expired() const { > + return (Type == Zero) || > + (Type == Finite && Time < std::chrono::steady_clock::now()); > + } > + bool operator==(const Deadline &Other) const { > + return (Type == Other.Type) && (Type != Finite || Time == Other.Time); > + } > + > +private: > + enum Type { Zero, Infinite, Finite }; > + > + Deadline(enum Type Type) : Type(Type) {} > + enum Type Type; > + std::chrono::steady_clock::time_point Time; > +}; > + > +/// Makes a deadline from a timeout in seconds. None means wait forever. > Deadline timeoutSeconds(llvm::Optional<double> Seconds); > +/// Wait once on CV for the specified duration. > +void wait(std::unique_lock<std::mutex> &Lock, std::condition_variable &CV, > + Deadline D); > /// Waits on a condition variable until F() is true or D expires. > template <typename Func> > LLVM_NODISCARD bool wait(std::unique_lock<std::mutex> &Lock, > std::condition_variable &CV, Deadline D, Func F) > { > - if (D) > - return CV.wait_until(Lock, *D, F); > - CV.wait(Lock, F); > + while (!F()) { > + if (D.expired()) > + return false; > + wait(Lock, CV, D); > + } > return true; > } > > @@ -73,7 +105,7 @@ public: > /// Destructor waits for all pending tasks to finish. > ~AsyncTaskRunner(); > > - void wait() const { (void) wait(llvm::None); } > + void wait() const { (void)wait(Deadline::infinity()); } > LLVM_NODISCARD bool wait(Deadline D) const; > // The name is used for tracing and debugging (e.g. to name a spawned > thread). > void runAsync(llvm::Twine Name, UniqueFunction<void()> Action); > > 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=326546&r1=326545&r2=326546&view=diff > > ============================================================================== > --- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp > (original) > +++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Fri Mar > 2 00:56:37 2018 > @@ -42,7 +42,8 @@ private: > TEST_F(TUSchedulerTests, MissingFiles) { > TUScheduler S(getDefaultAsyncThreadsCount(), > /*StorePreamblesInMemory=*/true, > - /*ASTParsedCallback=*/nullptr); > + /*ASTParsedCallback=*/nullptr, > + > /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero()); > > auto Added = testPath("added.cpp"); > Files[Added] = ""; > @@ -94,9 +95,11 @@ TEST_F(TUSchedulerTests, WantDiagnostics > // To avoid a racy test, don't allow tasks to actualy run on the > worker > // thread until we've scheduled them all. > Notification Ready; > - TUScheduler S(getDefaultAsyncThreadsCount(), > - /*StorePreamblesInMemory=*/true, > - /*ASTParsedCallback=*/nullptr); > + TUScheduler S( > + getDefaultAsyncThreadsCount(), > + /*StorePreamblesInMemory=*/true, > + /*ASTParsedCallback=*/nullptr, > + /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero()); > auto Path = testPath("foo.cpp"); > S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes, > [&](std::vector<DiagWithFixIts>) { Ready.wait(); }); > @@ -118,6 +121,28 @@ TEST_F(TUSchedulerTests, WantDiagnostics > EXPECT_EQ(2, CallbackCount); > } > > +TEST_F(TUSchedulerTests, Debounce) { > + std::atomic<int> CallbackCount(0); > + { > + TUScheduler S(getDefaultAsyncThreadsCount(), > + /*StorePreamblesInMemory=*/true, > + /*ASTParsedCallback=*/nullptr, > + /*UpdateDebounce=*/std::chrono::milliseconds(50)); > + auto Path = testPath("foo.cpp"); > + S.update(Path, getInputs(Path, "auto (debounced)"), > WantDiagnostics::Auto, > + [&](std::vector<DiagWithFixIts> Diags) { > + ADD_FAILURE() << "auto should have been debounced and > canceled"; > + }); > + std::this_thread::sleep_for(std::chrono::milliseconds(10)); > + S.update(Path, getInputs(Path, "auto (timed out)"), > WantDiagnostics::Auto, > + [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; }); > + std::this_thread::sleep_for(std::chrono::milliseconds(60)); > + S.update(Path, getInputs(Path, "auto (shut down)"), > WantDiagnostics::Auto, > + [&](std::vector<DiagWithFixIts> Diags) { ++CallbackCount; }); > + } > + EXPECT_EQ(2, CallbackCount); > +} > + > TEST_F(TUSchedulerTests, ManyUpdates) { > const int FilesCount = 3; > const int UpdatesPerFile = 10; > @@ -131,7 +156,8 @@ TEST_F(TUSchedulerTests, ManyUpdates) { > { > TUScheduler S(getDefaultAsyncThreadsCount(), > /*StorePreamblesInMemory=*/true, > - /*ASTParsedCallback=*/nullptr); > + /*ASTParsedCallback=*/nullptr, > + /*UpdateDebounce=*/std::chrono::milliseconds(50)); > > std::vector<std::string> Files; > for (int I = 0; I < FilesCount; ++I) { > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > -- Regards, Ilya Biryukov
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
