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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

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

SGTM. Note that this means we can't easily test this in LSP layer anymore. (We've got couple of components depending on time, maybe it is time we have a "mock" clock?)



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:182
+    Server.Server->profile(MT);
+    trace::recordMemoryUsage(MT, "clangd_server");
     return true;
----------------
sammccall wrote:
> 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.
> 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.

Added a `record` member to MemoryTree in D88413.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417