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