[PATCH] D75603: [clangd] Add instrumentation mode in clangd for metrics collection.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 marked 3 inline comments as done.
usaxena95 added inline comments.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1465
+    if (Opts.EnableInstrumentationMode)
+      (*Opts.RecordCCResults)(toCodeCompleteResult(Top));
+
----------------
kadircet wrote:
> can't we make use of the trace::Span instead ?
CMIIW. I believe with `trace::Span` we can send only JSON messages from clangd to the tool running it. This doesn't allow us to get access to internal DS of CodeCompleteFlow that are used along with Quality and Relevance signals (like Distances of file and scopes).
You can argue that all this information can be serialized as a JSON (including features derived from these internal DS) but this then must be done on clangd side (not on tools side).

IMO this gives more freedom to the tool to use and derive more features which makes experimentation easier.


================
Comment at: clang-tools-extra/clangd/CodeComplete.h:141
+  /// Ideally this must be disabled when being used by ClangdLSPServer.
+  bool EnableInstrumentationMode = false;
+  std::function<void(const CodeCompleteResult&)>* RecordCCResults;
----------------
sammccall wrote:
> I'm wondering if we can simplify this interface a bit. I'm not sure why we need another callback rather than just returning the CodeCompleteResult in the usual way.
> 
> Another option:
>  - if we invoke a callback for each completion instead of for the result set as a whole, we don't need to work out where to stash anything.
>  - the work done after `addCandidate` is pretty trivial, so invoking a callback there provides basically all the information about the result set. The Top-N truncation is probably something you'd rather *not* have for analysis.
>  - code completion always ends with the callback being invoked, so cross-result analysis can be done at that point.
> 
> So I think this could just be a single
> `std::function<void(const CodeCompletion&, const SymbolQualitySignals &, const SymbolRelevanceSignals &)>`.
> If it's non-null, addCandidate would call toCodeCompletion on the bundle and pass it to the callback at the end.
> why we need another callback rather than just returning the CodeCompleteResult in the usual way.
There are some data structures from CodeCompleteFlow referred to in Reference signals like `ScopeDistance` which were needed to compute distances. But think can be addressed in your suggested "per completion" callback approach.

> Another option: ...
I had given this approach some thought previously and had concerns about mapping the items to the final ranked items in the TopN result. But on a second thought we can completely ignore the final result (assuming no significant changes are done after `addCandidate`) and **score** and **rank** the results ourselves in the FlumeTool. 




================
Comment at: clang-tools-extra/clangd/CodeComplete.h:218
+    // Signals captured when instrumentation is enabled during code completion.
+    std::shared_ptr<SymbolQualitySignals> QualitySignals;
+    std::shared_ptr<SymbolRelevanceSignals> RelevanceSignals;
----------------
sammccall wrote:
> why shared rather than unique?
A not-so-proud hack to keep `Score` copyable (this is removed now).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75603