Add tests and fix bugs

Rebase, implement yaml bits (no tests yet)

Thanks, I'd seen these crashes but not manage to track down yet!

Thanks a lot for doing this!
I have some thoughts on how we can give it a nice API/more clearly separate it from FindTarget/improve internal structure.

address comments

Can you add a test for whatever this is fixing?

Yikes, thanks!

Thanks!

Thanks, this looks pretty solid!

In fact, never index the same file twice at all.

Thanks for looking into this feature!

(What broke this?)

Ah, this is a bit ugly... I'm not really happy with our representation of info extracted from the preprocessor, but that's something to tackle another time.

Thanks! This is a really good idea, but not without its risks :-) Sorry for not getting to this for a while!

Doh, I really thought we'd get away without an explicit recursion guard here.
But the example is compelling, so this seems like the right approach. Unfortunately, I think we're going to end up needing to add allocations too...

Yes, I was getting deja vu reading the description. Thanks though!

In D94293#2486468, @njames93 wrote:

How does this approach work with differing language standards. For example with an old implementation designed for c++14, string_view header won't exist. If the implementation is designed to work with c++17, including that header in c++14 mode will probably be ifdef... Error.

Thanks, this looks much better!

Ah, thanks for working on this!

I tried to sketch a couple of tetris-based mental models for why these hashtables might never fit inside an existing gap, leading us to stack our arena size (tetris field height) higher and higher.
This is predicated on the assumption that the index hashtables are the biggest single contiguous allocations in clangd, which I think is true. (It also explains why std::map fares better - each node is a separate allocation. Unfortunately, this is also why it's performance is terrible...)
Not sure which/either is model true, but I found it entertaining.

In D93452#2460646, @qchateau wrote:

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.

Seems fair enough to do something about this, though it's a bit sad we're doing it just because VSCode has bad UI.

Yup, as mentioned on D93393 this seems like a reasonable tradeoff to me. Thanks!

Sorry for the delay getting this landed, and thanks for the patch!
It'll be part of the clangd 12 release.

@dexonsmith thanks for sharing!
Some initial thoughts since abstracting outputs is something we're starting to care about too...

In D93683#2469553, @ArcsinX wrote:

In D93683#2468842, @sammccall wrote:

I'm not 100% sure this bug would block adding the equivalent change for fuzzyFind though - it'll affect documentSymbol but not code-completion IIUC (because of the workaround I mentioned above). And these symbols are pretty rare.

With the similar change for fuzzyFind(), WorkspaceSymbols.Macros test fails. But maybe I can create one more review for this and we will discuss it there.

Thanks, this looks good, just a couple of nits.

In D93452#2467471, @njames93 wrote:

I feel like we need some kind of entry for this in release notes given how much of an issue it is.

In D93726#2468855, @qchateau wrote:

LGTM

Small nits:

• operator() is not const but Next is mutable, seems to me you intended to have operator() as const

Oops, I originally did plan to make operator() const, but then decided it'd be clearer for the caller to make PeriodicThrottler mutable instead. (Because pretending it doesn't have state is confusing).
Removed mutable from Next.

In D93683#2467631, @ArcsinX wrote:

I also want to propose the similar patch for fuzzyFind() (as a part of this patch or a separate one), but I faced the following problem:

Test.cpp

#define FOO 1

This example creates empty dynamic index for main file symbols and static index for preamble symbols (FOO). The location of FOO is inside Test.cpp, so (according to the logic that the static index could be stale) we should toss FOO from the static index (i.e. preamble symbols), but this behaviour is incorrect... Any advice?

In D93452#2467046, @qchateau wrote:

I did not use the CMke option as the default value for the command line option: IMO this CMake option is only useful if you encounter build problems related to malloc_trim, so malloc_trim must not be part of the code when you disable it through CMake.

This seems a bit too conservative to me, I wouldn't expect anyone to ever encounter such build problems (e.g. glibc so old that malloc_trim doesn't exist, or header exists but library doesn't), so I'd add a workaround if they actually arose.
The other value of the CMake option is being able to disable the *default* behavior if you use another allocator, so that you don't have to ask all users to pass a custom flag.

It's up to you - it's easy for me/others to land them on your behalf. Having your own commit access gives you more control, ability to deal with build breakages quickly, fix trivial things without review etc.

I've put a version of the reading part in D93653. This seems a bit less invasive to me - I'm still not sure if it's worth the readability hit though...

Thanks, this version seems just as clear as the original to me, and fewer allocations is nice :-)

Thanks, LG - want me to land it?

Thanks, this looks like a good idea to me, but some quibbles with the details:

• for any string, we statically know whether it's path or WD-relative
• we should make sure all the IO gets cached

Awesome! Some nits on naming and stuff, but the only real substantial question for me is the exact availability/behavior of the command-line flag. Even there, as long as the default behavior is to trim when build with glibc, the details are less important.
Let me know when you want me to land it!

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. My reasoning is:

• Once we are injecting the cleanup function from ClangdMain, that's not actually a crazy place to put this kind of system-level code, behind #ifdefs
• The abstraction costs us something: we can't skip all the locking etc for memory cleanups if it's a no-op on our platform
• We're putting an abstraction around one implementation without any reason to believe other implementations need this treatment or can use a similar interface. This is likely to turn out to be a poor abstraction, and llvm/Support isn't the easiest place to iterate on it.

Thanks! Changes to ClangdServer are definitely good, I'm less sure about renderTUAction.

In D93553#2463577, @qchateau wrote:

Looks good to me, it removes a lot of duplication and corner cases.

I'd have printed the tag for reference and pointers as well (class Foo* instead of Foo*). I don't think Foo* is much more understandable than Foo, so we decide that printing class Foo is easier to understand for the user, we should print class Foo* as well.
But I agree it's even less idiomatic, so I won't argue with your choice.

Thanks, this looks much closer to something we can land.
Bunch of nits :-)

In D93531#2463126, @njames93 wrote:

In D93531#2463052, @sammccall wrote:

This adds a bit of complexity, making the code here a fair amount harder to follow and verify the correctness of.

• Do we have evidence that these allocations are causing a problem? (e.g. do we observe a significant decrease in RSS after the patch)? Naively I would expect these allocations to be basically unimportant compared to those of the JSON objects themselves.(And I don't particularly expect either of them to be significant - the comment on the other review was really just "0 probably isn't the right arg to malloc_trim if there's any allocation going on").

It's not causing problems per say. but given the incoming json messages can contain a whole file plus things like escape chars. its wise to allocate a buffer that will grow to the largest json it receives but never shrink.