This revision was automatically updated to reflect the committed changes.
Closed by commit rL333737: [clangd] Keep only a limited number of idle ASTs in
memory (authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D47063
ilya-biryukov added inline comments.
Comment at: clangd/TUScheduler.h:66
+ std::chrono::steady_clock::duration UpdateDebounce,
+ ASTRetentionPolicy RetentionPolicy = {});
~TUScheduler();
sammccall wrote:
> ilya-biryukov wrote:
> > sam
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clangd/TUScheduler.h:66
+ std::chrono::steady_clock::duration UpdateDebounce,
+ ASTRetentionPolicy RetentionPolicy = {});
~
ilya-biryukov updated this revision to Diff 149075.
ilya-biryukov added a comment.
- Fixed formatting
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47063
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/TUScheduler
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.h:143
+std::shared_ptr
+buildPreamble(PathRef FileName, CompilerInvocation &CI,
+ std::shared_ptr OldPreamble,
sammccall wrote:
> nit: i think filename here is only used for logging,
ilya-biryukov updated this revision to Diff 149073.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.
- Added a unit test
- Address review comments
- Add ASTRetentionPolicy param to ClangdServer
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47063
F
sammccall added a comment.
Impl looks good.
Is there a way we can reasonably test this? (specifically that we don't just
store all the ASTs forever)
Comment at: clangd/ClangdUnit.h:143
+std::shared_ptr
+buildPreamble(PathRef FileName, CompilerInvocation &CI,
+ std
ilya-biryukov added a comment.
There are still a few nits I haven't addressed, but the other big change is now
there: removing ASTBuilder, replacing it with free-standing functions. I'd say
I liked the previous code better, since the use sites were a bit smaller and
the functions have ridiculou
ilya-biryukov updated this revision to Diff 148824.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
- s/Params/Policy
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47063
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/TUScheduler.cp
ilya-biryukov added inline comments.
Comment at: clangd/TUScheduler.cpp:71
+
+ /// Update the function used to compute the value.
+ void update(std::function()> ComputeF);
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > I think I understand t
ilya-biryukov updated this revision to Diff 148822.
ilya-biryukov added a comment.
- Remove ASTBuilder, make everything a helper function
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47063
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/TUScheduler.cpp
clangd/
ilya-biryukov updated this revision to Diff 148820.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.
- Address review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47063
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/TUSch
sammccall added a comment.
The cache looks way simpler now, thank you!
As discussed offline, flattening ASTBuilder right into ASTWorker still seems
like a good idea to me, but happy with what you come up with there.
Comment at: clangd/TUScheduler.cpp:71
+
+ /// Update the fun
ilya-biryukov added a comment.
I have addressed the comments regarding the cache implementation. ASTBuilder
ones are still pending, but I would appreciate the feedback on how
`TUScheduler.cpp` looks like.
Comment at: clangd/ClangdUnit.h:132
-/// Manages resources, required
ilya-biryukov updated this revision to Diff 148599.
ilya-biryukov added a comment.
- Rebase, fix merge conflicts
- Simpler implemenataion of the Cache
- s/IdleASTs/ASTCache
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47063
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
sammccall added a comment.
Thanks, this looks a lot better!
There do seem to be a lot of little classes that exist exactly one-per-TU
(ASTWorker, ASTBuilder, CachedAST, to a lesser extent ParsedAST) and I'm not
sure the balance of responsibilities is quite right. Some comments below.
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.h:132
-/// Manages resources, required by clangd. Allows to rebuild file with new
-/// contents, and provides AST and Preamble for it.
-class CppFile {
+/// A helper class that handles building preambles and AST
ilya-biryukov updated this revision to Diff 148355.
ilya-biryukov added a comment.
- Reimplemented LRU cache with shared_ptr and weak_ptr.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47063
Files:
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/TUScheduler.cpp
clangd
sammccall added a comment.
Having taken a closer look, I think the cache can be simplified/separated a bit
more cleanly by returning shared pointers and not allowing lookups, instead
restoring limited ownership in CppFile...
Happy to discuss more, esp if you might disagree :)
===
sammccall added a comment.
(Haven't reviewed the code yet, just design stuff)
I'm tempted to push back on the idea that we need to store any - "if it's fast
enough for code complete, it's fast enough for GTD". But that's probably
oversimplifying - we don't force reuse of a stable preamble for GT
malaperle added subscribers: arphaman, malaperle.
malaperle added a comment.
In https://reviews.llvm.org/D47063#1104417, @ilya-biryukov wrote:
> Maybe we can even store enough information to not need the AST for navigation
> at all and build it only for features like refactorings.
> @sammccall,
ilya-biryukov added a comment.
Another alternative that I've considered was evicting the ASTs from memory
after a certain period of time, e.g. after 30 seconds of inactivity for some
file. This might be simpler and also cover the use-case of speeding up multiple
code navigation requests (findDe
ilya-biryukov created this revision.
ilya-biryukov added a reviewer: sammccall.
Herald added subscribers: jkorous, MaskRay, ioeric, javed.absar, klimek.
After this commit, clangd will only keep the last 3 accessed ASTs in
memory. Preambles for each of the opened files are still kept in
memory to m
23 matches
Mail list logo
