This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Make sure project-aware index is up-to-date for estimateMemoryUsage()
AbandonedPublic

Authored by kbobyrev on Nov 26 2020, 6:32 PM.

Details

Reviewers
sammccall

Diff Detail

Event Timeline

kbobyrev created this revision.Nov 26 2020, 6:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2020, 6:32 PM
kbobyrev requested review of this revision.Nov 26 2020, 6:32 PM

Why do we want to do this?
The existing estimate covers the existing loaded data.
This change prompts it to load data that we think we might need soon before estimating, but I'm not sure why that's a good thing.

(actually I don't think it'll work as if it causes new indexes to be loaded, they'll load asynchronously and report 0 here)

Hm, yes, this makes sense. I just found it awkward that memory usage estimate will spike right after the first request but that's actually the reality (loading the index is deferred until the first actual index request). My confusion here is that I don't really understand whether deferring the index loading is a good thing because we're practically moving the delay from the warm-up to the working state which is suboptimal. I was trying to cause the index to load in the initialization stage for D92198 but couldn't figure out where exactly the config is loaded and what could be the best place for triggering index loading.

kbobyrev abandoned this revision.Nov 28 2020, 10:28 AM

Hm, yes, this makes sense. I just found it awkward that memory usage estimate will spike right after the first request but that's actually the reality (loading the index is deferred until the first actual index request).

Exactly. The good news is: I don't think we have any particular use case for probing the memory usage right after startup.

My confusion here is that I don't really understand whether deferring the index loading is a good thing because we're practically moving the delay from the warm-up to the working state which is suboptimal.

It's not great that we offer partial functionality without signaling this somehow, and we could consider making this more transparent.
However this happens in lots of places in clangd, especially w.r.t index (completion before preamble, background index async load, actual background indexing itself, the cached-build-artifacts we have internally at google) and in every case feedback has been positive when we introduced it. Blocking the user for any one feature is rarely a good trade, if we can recover even half-gracefully instead.

I was trying to cause the index to load in the initialization stage for D92198 but couldn't figure out where exactly the config is loaded and what could be the best place for triggering index loading.

Fair point, will have a look at that. Forcing an index load might be a nice side-effect of this patch, but I think we can find a better way to do it.

I was trying to cause the index to load in the initialization stage for D92198 but couldn't figure out where exactly the config is loaded and what could be the best place for triggering index loading.

Fair point, will have a look at that. Forcing an index load might be a nice side-effect of this patch, but I think we can find a better way to do it.

I suppose the most natural place would be under ClangdServer::addDocument, but we don't have any nice way to trigger this warmup conceptually. All we can do is perform some other index operation that'll trigger index loading as a side-effect, which makes it quite weird to trigger in any "direct" place :/