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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
