[PATCH] D55770: [clangd] BackgroundIndex rebuilds symbol index periodically.

This revision was automatically updated to reflect the committed changes. Closed by commit rL349496: [clangd] BackgroundIndex rebuilds symbol index periodically. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINCE LAST ACTION https:

[PATCH] D55770: [clangd] BackgroundIndex rebuilds symbol index periodically.

ioeric updated this revision to Diff 178676. ioeric marked an inline comment as done. ioeric added a comment. - Add comment about double-check of ShouldStop. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55770/new/ https://reviews.llvm.org/D55770 F

[PATCH] D55770: [clangd] BackgroundIndex rebuilds symbol index periodically.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: clangd/index/Background.h:112 + const size_t BuildIndexPeriodMs; + std::atomic SymbolsUpdatedSinceLastIndex; + std::mutex IndexMu; --

[PATCH] D55770: [clangd] BackgroundIndex rebuilds symbol index periodically.

kadircet added inline comments. Comment at: clangd/index/Background.h:112 + const size_t BuildIndexPeriodMs; + std::atomic SymbolsUpdatedSinceLastIndex; + std::mutex IndexMu; kadircet wrote: > ioeric wrote: > > kadircet wrote: > > > We already have a mutex and

[PATCH] D55770: [clangd] BackgroundIndex rebuilds symbol index periodically.

kadircet added inline comments. Comment at: clangd/index/Background.cpp:352 + std::unique_lock Lock(IndexMu); + if (ShouldStop) +break; Is double checking really necessary? I suppose it is for the case that we miss the notification, if that's t

[PATCH] D55770: [clangd] BackgroundIndex rebuilds symbol index periodically.

ioeric marked an inline comment as done. ioeric added inline comments. Comment at: clangd/index/Background.h:112 + const size_t BuildIndexPeriodMs; + std::atomic SymbolsUpdatedSinceLastIndex; + std::mutex IndexMu; kadircet wrote: > We already have a mutex and

[PATCH] D55770: [clangd] BackgroundIndex rebuilds symbol index periodically.

kadircet added inline comments. Comment at: clangd/index/Background.h:112 + const size_t BuildIndexPeriodMs; + std::atomic SymbolsUpdatedSinceLastIndex; + std::mutex IndexMu; We already have a mutex and cv, maybe get rid of this one signal the CV whenever we

[PATCH] D55770: [clangd] BackgroundIndex rebuilds symbol index periodically.

ioeric marked an inline comment as done. ioeric added a comment. In D55770#1333271 , @dblaikie wrote: > (from the peanut gallery): Usually what I'd expect here is "reindexing will > occur X milliseconds after a write, covering anything written up until th

[PATCH] D55770: [clangd] BackgroundIndex rebuilds symbol index periodically.

dblaikie added a comment. (from the peanut gallery): Usually what I'd expect here is "reindexing will occur X milliseconds after a write, covering anything written up until that point" - so it doesn't reindex unnecessarily if the files are unmodified/sitting idle, and it doesn't thrash reindexi

[PATCH] D55770: [clangd] BackgroundIndex rebuilds symbol index periodically.

kadircet added inline comments. Comment at: clangd/index/Background.cpp:464 + if (BuildIndexPeriodMs > 0) +SymbolsUpdatedSinceLastIndex = true; + else why not simply check if `BuildIndexPeriodMs` has passed and issue rebuild here? That way we won't spawn a

[PATCH] D55770: [clangd] BackgroundIndex rebuilds symbol index periodically.

ioeric created this revision. ioeric added reviewers: ilya-biryukov, kadircet. Herald added subscribers: cfe-commits, jfb, arphaman, jkorous, MaskRay. Currently, background index rebuilds symbol index on every indexed file, which can be inefficient. This patch makes it only rebuild symbol index p