[PATCH] D88417: [clangd] Record memory usages after each notification

[Kadir Cetinkaya via Phabricator via cfe-commits]

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