kadircet requested review of this revision.
kadircet added a comment.
Bad news, I was testing this with remote-index, hence background-index was
turned off. Unfortunately traversing all of the slabs in `FileSymbols` takes
quite a while in this case (~15ms for LLVM).
I don't think it is feasible to do this on every notification now, as this
implies an extra 15ms latency for interactive requests like code
completion/signature help due to the delay between didChange notification and
codeCompletion request.
> We should watch the timing here carefully and consider guarding it - apart
> from the minimum time interval we discussed, we could have a check whether
> metric tracing is actually enabled in a meaningful way.
I've also added early exit for non-tracing case. But I think we should still
change this to be periodic or once every N calls. WDYT?
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:182
+ Server.Server->profile(MT);
+ trace::recordMemoryUsage(MT, "clangd_server");
return true;
----------------
sammccall wrote:
> (Sorry, I suspect we discussed this and I forgot)
> Is there a reason at this point to put knowledge of the core metric in
> trace:: rather than define it here locally in ClangdLSPServer?
> (Sorry, I suspect we discussed this and I forgot)
Not really.
> Is there a reason at this point to put knowledge of the core metric in
> trace:: rather than define it here locally in ClangdLSPServer?
`ClangdLSPServer` didnt feel like the appropriate place for that logic.
Moreover other embedders of ClangdServer could benefit from traversal logic if
it is defined in a lower level than ClangdLSPServer.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:168
elog("Notification {0} before initialization", Method);
- else if (Method == "$/cancelRequest")
+ return true;
+ }
----------------
sammccall wrote:
> this change is a bit puzzling - makes it look like there are some cases where
> we specifically want/don't want to record. why?
it was to ensure we have a `ClangdServer` instance we can query for memory
usage.
will revert as moving profiling into `happy case` makes it obselete.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88417/new/
https://reviews.llvm.org/D88417
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
