- User Since
- Dec 9 2020, 3:13 PM (26 w, 2 d)
Apr 15 2021
Have you guys been giving some thoughts to that patch ? I've been using it in my daily work since I submitted the patch, and I'd not go back
Mar 4 2021
Mar 3 2021
fix bad arc diff
rebase on main, fix formatting
Mar 2 2021
I am much more afraid of providing bad results than I am afraid of degrading performance. I mean, eventually the "real" preamble is built, and the results are just as correct as before, but it may yield incorrect results until we invalidate the AST.
That is especially the case as I'm not very experienced in compiler design and I don't have a deep understanding of what happens behind the curtains when using a PCH.
There are probably cases where we degrade perf, but overall my experience with this patch is overwhelmingly positive, especially for "long coding sessions".
Feb 26 2021
- [clangd] make more compile commands compatible
- [clangd] ignore incompatible preamble
Feb 25 2021
The hardcoded 5 premables can indeed be changed, I did not want to waste time on coding a configuration logic at such an early stage.
Feb 24 2021
Jan 25 2021
I applied your suggestion as I agree with both. I chose to use Opts.AsyncThreadsCount instead of a hard-coded constant. This way the "formatting speed" will grow as the user allow more parallelism, which IMO makes sense.
- [clangd] fix nits
Jan 17 2021
You're right, I probably got carried away for the sake of completeness. Allocating extra memory for a feature nobody is going to use is definitely not worth it. Though maybe we can take advantage of what's already been done and if we remove the RevRefs map, and pay the full price of iterating all refs when refersTo is called, we still have this feature. Although slower, it shouldn't take more than a few 100's of milliseconds even on a big project. Which means the feature is usable, at least for most small to medium sized projects, at no extra cost for people who do not use it.
Jan 14 2021
If you look back at my original comment, you'll notice I originally trimmed the memory after FileSymbols::buildIndex which seemed to be the "end of the peak" memory usage, the goal was to counteract exactly what you describe: a huge memory usage when clangd warms-up and parses a lot of files in parallel.
I wonder if we can find a middle ground. The solution we finally adopted also has the advantage of being less intrusive, which is definitely great considering the nature of the issue
Jan 11 2021
Hey, don't worry about the delay, I won't have as much time on my hands anymore anyway.
Jan 9 2021
New year's ping ?
Jan 7 2021
Ah ! It's been done already !
Dec 30 2020
- Fix keep preamble command line option
- Fix tests
Dec 28 2020
Dec 27 2020
Note: Build fails due to clang-tidy warnings in tests. I chose to keep the naming consistent with what's already in place rather than fix the clang-tidy warning.
- Smaller reversed refs memory usage
- Fix Dex::estimateMemoryUsage
Dec 26 2020
- Fix tests
Dec 22 2020
- operator() is not const but Next is mutable, seems to me you intended to have operator() as const
- memory orders for the atomic operations can probably be downgraded to std::memory_order_acquire/std::memory_order_acq_rel. I think the first load can even be relaxed but that I'm always careful with these
Dec 21 2020
The log is back, I renamed the CMake option and the command line option and reduced the number of preprocessor directives.
I chose not to modify the files for the gn build system, I'd rather not break it.
I guess I'll need to ask for commit access
- dont capitalize first letter of status
Dec 20 2020
Here is the latest version, I believe it should mostly look like what you expect. Let me know if there is anything that you'd like me to change, it's totally fine to have a few more iterations on this diff.
- Move platform specific code to ClangdMain
- Generic memory cleanup callback in ClangdLSPServer
Sorry to mess you around like this, but I think I might have jumped the gun in asking this to be abstracted away in Process.h.
No problem, there is not much code anyway. The thought process behind the "right" solution is by far the most important.
Interestingly clangd held rock solid at 1.0GB recorded memory usage throughout.
Dec 19 2020
Dec 18 2020
- Fix clang-tidy errors
Looks good to me, it removes a lot of duplication and corner cases.
- Add MallocTrim to llvm::sys::Process
- Implement malloc trimming in ClangdLSPServer
Fix ExpandAutoType to not expand undeduced auto
You can land this if it is still fine after my final update.
- Verify documentation in hover test on 'auto'
- Fixed comments
- Converted raw string to string to avoid trailing whitespace
Dec 17 2020
I updated this diff according to your review. There are a few things that may still need change but I need your input, see my other comments :)
Periodic malloc_trim seems like it solves a real problem, one I don't personally fully understand but we may never do - I feel convinced we should use it anyway. It probably has some overhead, who knows how much.
It does indeed look like facing the problem from the wrong side (fight the end result instead of finding the root cause) but at that point it does solve an annoying problem. As I investigated this, I found that replacing DenseMap and StringMap wtih std::map mitigates the issue. I think allocating huge chunks of contiguous memory temporarily make this issue worse. Not very useful, but hey, I'm just sharing what I observed. As I previously said, resolving this problem by changing containers or allocators is a lot of work and is not guaranteed to actually solve the issue.
Use malloc_trim only if available
Side note is this memory failing to free behaviour observed on windows?
We can't easily add new SymbolKinds (it's an enum we don't own, which we should fix, but that's a yak-shave). But TypeAlias almost fits these roles for auto.
- Rebase on master after D92041
- Remove the usage of the "Documentation" field
- Use the TypeAlias Kind on auto and decltype
- Move code related to hover on this in a new function
- Update hover on this to be consistent with this change
Dec 16 2020
So I'm leaning towards defining these behaviors for pointers and containers rather than templates per se. There's a nice symmetry with the raw pointer and array types that this patch.
(We have some precedent and code for detecting smart pointers, see getPointeeType() in FindTarget.cpp.)
Dec 15 2020
Ah good catch, I agree this is the behavior we want.
Dec 14 2020
Dec 11 2020
I've updated the patch with your latest comments.
- Reintroduce test with a FIXME comment
- locateSymbolForType returns a vector instead of an optional
- Remove the patches affecting hover
- Add tests in ASTTests.cpp to test the behavior of getDeducedType for auto and decltype
- Add missing comment
- Create a locateSymbolForType function (which uses targetDecl)
- Remove the unit tests that were testing undesirable behaviors
Dec 10 2020
Not sure what's wrong with the unit test "x64 windows > LLVM.CodeGen/XCore::threads.ll", I don't see how it's related to my patch
I've fixed the failing unittests, added some more
to test the new behavior, and fixed some bugs in
the new code.