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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D88417#2319307 <https://reviews.llvm.org/D88417#2319307>, @kadircet wrote:

> 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.

Yeah, 15ms is a lot.
Staring at the code, I think this is our unfortunate index allocation strategy: for each source file in the project, we have one slab allocator for symbols, one for refs, one for relations...
The factor of 3 is sad but the factor of 10000 is what's killing us :-)

So we should fix that someday (DBMS?) but for now, let's back off to measuring once every several minutes.

The main problem I see is that we're almost guaranteed to be "unlucky" with the sample that way.
e.g. delay = 5 min. On startup there's some notification (initialized? didOpen?) that happens before any of the serious allocation and resets the counter.
So we won't have a realistic memory usage until we hit the 5 minute mark and profile again. If many sessions are <5min then our averages are going to be way off.

I can't think of a notification that is ubiquitous but only fires after serious work has happened. (Well, publishDiagnostics, but that goes in the wrong direction and so runs on the wrong thread).

What about something like a 5 minute throttle, but have ClangdLSPServer's constructor set the timestamp to now+1 minute? (Without profiling)



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:152
+  trace::Span Tracer(Detailed ? "ProfileDetailed" : "ProfileBrief");
+  // Exit early if tracing is disabled.
+  if (!Tracer.Args)
----------------
I was about to complain that this doesn't work, but it actually does...

This wasn't the intended design of trace/span :-(
The idea was that `Args` would be null if the tracer dynamically wasn't interested in event details (e.g. an implementation like CSVMetricsTracer that doesn't record event payloads, or a sampling tracer).
In this case !Args would have false positives.

Do you mind adding an explicit trace::enabled() method to Trace.h instead, to be more explicit? I'd like to fix the Trace api.
(Else leave as-is and I can do this as a followup)


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:182
+    Server.Server->profile(MT);
+    trace::recordMemoryUsage(MT, "clangd_server");
     return true;
----------------
kadircet wrote:
> 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.
(Sorry, I guess D88413 was really the place for this discussion)

> ClangdLSPServer didnt feel like the appropriate place for that logic.

To me, putting this in ClangdLSPServer feels like "this isn't part of the central mission here, it could be split out for coherence/reuse".
Whereas putting it in support/Trace feels like "this is a layering violation".

> other embedders of ClangdServer could benefit from traversal logic

If exporting memorytree->metric with the path as the label is something we want to reuse, that could be a function in MemoryTree.h (taking the metric as parameter) or we can include the generic traversal function you proposed earlier. Even though they're both in Support, layering MemoryTree above Tracer seems more appropriate than the other way around.
My instinct is this is premature generalization and having a traversal in each embedder is fine, though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88417/new/

https://reviews.llvm.org/D88417