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

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 created this revision.
usaxena95 added a reviewer: sammccall.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

This patch adds an instrumentation mode for clangd (enabled by
corresponding option in cc_opts).
If this mode is enabled then user can specify callbacks to run on the
final code completion result.

Moreover the CodeCompletion::Score will contain the detailed Quality and
Relevance signals used to compute the score when this mode is enabled.
These are required because we do not any place in which the final
candidates (scored and sorted) are available along with the above
signals. The signals are temporary structures in `addCandidate`.

The callback is needed as it gives access to many data structures that
are internal to CodeCompleteFlow and are available once Sema has run. Eg:
ScopeDistnace and FileDistance.

If this mode is disabled (as in default) then Score would just contain 2
shared pointers (null). Thus cost(memory/time) increase for the default
mode would be fairly cheap and insignificant.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75603

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeComplete.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -29,6 +29,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 
 namespace clang {
@@ -1041,6 +1042,31 @@
   EXPECT_THAT(Completions, Contains(Named("TT")));
 }
 
+TEST(CompletionTest, EnableInstrumentationMode) {
+  CodeCompleteResult CapturedResult;
+  std::function ResultRecorder =
+  [&CapturedResult](const CodeCompleteResult &Result) -> void {
+CapturedResult = Result;
+  };
+  CodeCompleteOptions cc_opts;
+  cc_opts.EnableInstrumentationMode = true;
+  cc_opts.RecordCCResults = &ResultRecorder;
+
+  auto Results =
+  completions("int xyz; int a = xy^", /*IndexSymbols=*/{}, cc_opts);
+  EXPECT_EQ(Results.Completions.size(), CapturedResult.Completions.size());
+  ASSERT_GT(CapturedResult.Completions.size(), 0);
+  EXPECT_TRUE(CapturedResult.Completions.front().Score.QualitySignals);
+  EXPECT_TRUE(CapturedResult.Completions.front().Score.RelevanceSignals);
+}
+
+TEST(CompletionTest, DisableInstrumentationModeByDefault) {
+  auto Results = completions("int xyz; int a = xy^");
+  ASSERT_GT(Results.Completions.size(), 0);
+  EXPECT_FALSE(Results.Completions.front().Score.QualitySignals);
+  EXPECT_FALSE(Results.Completions.front().Score.RelevanceSignals);
+}
+
 SignatureHelp signatures(llvm::StringRef Text, Position Point,
  std::vector IndexSymbols = {}) {
   std::unique_ptr Index;
Index: clang-tools-extra/clangd/CodeComplete.h
===
--- clang-tools-extra/clangd/CodeComplete.h
+++ clang-tools-extra/clangd/CodeComplete.h
@@ -19,6 +19,7 @@
 #include "Logger.h"
 #include "Path.h"
 #include "Protocol.h"
+#include "Quality.h"
 #include "index/Index.h"
 #include "index/Symbol.h"
 #include "index/SymbolOrigin.h"
@@ -29,12 +30,15 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
+#include 
 #include 
+#include 
 
 namespace clang {
 class NamedDecl;
 namespace clangd {
 struct PreambleData;
+struct CodeCompleteResult;
 
 struct CodeCompleteOptions {
   /// Returns options that can be passed to clang's completion engine.
@@ -129,6 +133,13 @@
 /// Always use text-based completion.
 NeverParse,
   } RunParser = ParseIfReady;
+
+  /// Enables special instrumentation mode that allows capturing various
+  /// internal structures used by clangd during code completion. Eg: Symbol
+  /// quality and relevance signals of all the candidates in the output.
+  /// Ideally this must be disabled when being used by ClangdLSPServer.
+  bool EnableInstrumentationMode = false;
+  std::function* RecordCCResults;
 };
 
 // Semi-structured representation of a code-complete suggestion for our C++ API.
@@ -202,6 +213,10 @@
 // Relevance describes how well this candidate matched the query.
 // e.g. symbols from nearby files have higher relevance.
 float Relevance = 0.f;
+
+// Signals captured when instrumentation is enabled during code completion.
+std::shared_ptr QualitySignals;
+std::shared_ptr RelevanceSignals;
   };
   Scores Score;
 
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -65,7 +65,9 @@
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include 
+#incl

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

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 248160.
usaxena95 added a comment.

Addressed linter issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75603

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeComplete.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -29,6 +29,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 
 namespace clang {
@@ -1041,6 +1042,31 @@
   EXPECT_THAT(Completions, Contains(Named("TT")));
 }
 
+TEST(CompletionTest, EnableInstrumentationMode) {
+  CodeCompleteResult CapturedResult;
+  std::function ResultRecorder =
+  [&CapturedResult](const CodeCompleteResult &Result) -> void {
+CapturedResult = Result;
+  };
+  CodeCompleteOptions Opts;
+  Opts.EnableInstrumentationMode = true;
+  Opts.RecordCCResults = &ResultRecorder;
+
+  auto Results =
+  completions("int xyz; int a = xy^", /*IndexSymbols=*/{}, Opts);
+  EXPECT_EQ(Results.Completions.size(), CapturedResult.Completions.size());
+  ASSERT_GT(CapturedResult.Completions.size(), 0);
+  EXPECT_TRUE(CapturedResult.Completions.front().Score.QualitySignals);
+  EXPECT_TRUE(CapturedResult.Completions.front().Score.RelevanceSignals);
+}
+
+TEST(CompletionTest, DisableInstrumentationModeByDefault) {
+  auto Results = completions("int xyz; int a = xy^");
+  ASSERT_GT(Results.Completions.size(), 0);
+  EXPECT_FALSE(Results.Completions.front().Score.QualitySignals);
+  EXPECT_FALSE(Results.Completions.front().Score.RelevanceSignals);
+}
+
 SignatureHelp signatures(llvm::StringRef Text, Position Point,
  std::vector IndexSymbols = {}) {
   std::unique_ptr Index;
Index: clang-tools-extra/clangd/CodeComplete.h
===
--- clang-tools-extra/clangd/CodeComplete.h
+++ clang-tools-extra/clangd/CodeComplete.h
@@ -19,6 +19,7 @@
 #include "Logger.h"
 #include "Path.h"
 #include "Protocol.h"
+#include "Quality.h"
 #include "index/Index.h"
 #include "index/Symbol.h"
 #include "index/SymbolOrigin.h"
@@ -29,12 +30,15 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
+#include 
 #include 
+#include 
 
 namespace clang {
 class NamedDecl;
 namespace clangd {
 struct PreambleData;
+struct CodeCompleteResult;
 
 struct CodeCompleteOptions {
   /// Returns options that can be passed to clang's completion engine.
@@ -129,6 +133,13 @@
 /// Always use text-based completion.
 NeverParse,
   } RunParser = ParseIfReady;
+
+  /// Enables special instrumentation mode that allows capturing various
+  /// internal structures used by clangd during code completion. Eg: Symbol
+  /// quality and relevance signals of all the candidates in the output.
+  /// Ideally this must be disabled when being used by ClangdLSPServer.
+  bool EnableInstrumentationMode = false;
+  std::function *RecordCCResults;
 };
 
 // Semi-structured representation of a code-complete suggestion for our C++ API.
@@ -202,6 +213,10 @@
 // Relevance describes how well this candidate matched the query.
 // e.g. symbols from nearby files have higher relevance.
 float Relevance = 0.f;
+
+// Signals captured when instrumentation is enabled during code completion.
+std::shared_ptr QualitySignals;
+std::shared_ptr RelevanceSignals;
   };
   Scores Score;
 
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -65,7 +65,9 @@
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include 
+#include 
 #include 
+#include 
 
 // We log detailed candidate here if you run with -debug-only=codecomplete.
 #define DEBUG_TYPE "CodeComplete"
@@ -1458,6 +1460,10 @@
 // Merge Sema and Index results, score them, and pick the winners.
 auto Top =
 mergeResults(Recorder->Results, IndexResults, /*Identifiers*/ {});
+
+if (Opts.EnableInstrumentationMode)
+  (*Opts.RecordCCResults)(toCodeCompleteResult(Top));
+
 return toCodeCompleteResult(Top);
   }
 
@@ -1660,6 +1666,12 @@
? Scores.Total / Relevance.NameMatch
: Scores.Quality;
 
+if (Opts.EnableInstrumentationMode) {
+  Scores.QualitySignals = std::make_shared(Quality);
+  Scores.RelevanceSignals =
+  std::make_shared(Relevance);
+}
+
 dlog("CodeComplete: {0} ({1}) = {2}\n{3}{4}\n", First.Name,
  llvm::to_string(Origin), Scores.Total, llvm::to_string(Quality),
  

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/CodeComplete.h:141
+  /// Ideally this must be disabled when being used by ClangdLSPServer.
+  bool EnableInstrumentationMode = false;
+  std::function* RecordCCResults;

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`.
If it's non-null, addCandidate would call toCodeCompletion on the bundle and 
pass it to the callback at the end.



Comment at: clang-tools-extra/clangd/CodeComplete.h:218
+// Signals captured when instrumentation is enabled during code completion.
+std::shared_ptr QualitySignals;
+std::shared_ptr RelevanceSignals;

why shared rather than unique?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75603



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1465
+if (Opts.EnableInstrumentationMode)
+  (*Opts.RecordCCResults)(toCodeCompleteResult(Top));
+

can't we make use of the trace::Span instead ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75603



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 248233.
usaxena95 marked 3 inline comments as done.
usaxena95 added a comment.

Changed to invoke callback on all code completion items.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75603

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeComplete.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -29,7 +29,9 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -1041,6 +1043,24 @@
   EXPECT_THAT(Completions, Contains(Named("TT")));
 }
 
+TEST(CompletionTest, EnableInstrumentationMode) {
+  std::vector RecordedCompletions;
+  std::function
+  ResultRecorder =
+  [&RecordedCompletions](const CodeCompletion &CC,
+ const SymbolQualitySignals &,
+ const SymbolRelevanceSignals &) -> void {
+RecordedCompletions.push_back(CC);
+  };
+  CodeCompleteOptions Opts;
+  Opts.RecordCCResult = &ResultRecorder;
+
+  completions("int xy1, xy2; int a = xy^", /*IndexSymbols=*/{}, Opts);
+  EXPECT_THAT(RecordedCompletions,
+  UnorderedElementsAre(Named("xy1"), Named("xy2")));
+}
+
 SignatureHelp signatures(llvm::StringRef Text, Position Point,
  std::vector IndexSymbols = {}) {
   std::unique_ptr Index;
Index: clang-tools-extra/clangd/CodeComplete.h
===
--- clang-tools-extra/clangd/CodeComplete.h
+++ clang-tools-extra/clangd/CodeComplete.h
@@ -19,6 +19,7 @@
 #include "Logger.h"
 #include "Path.h"
 #include "Protocol.h"
+#include "Quality.h"
 #include "index/Index.h"
 #include "index/Symbol.h"
 #include "index/SymbolOrigin.h"
@@ -29,12 +30,14 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
+#include 
 #include 
 
 namespace clang {
 class NamedDecl;
 namespace clangd {
 struct PreambleData;
+struct CodeCompletion;
 
 struct CodeCompleteOptions {
   /// Returns options that can be passed to clang's completion engine.
@@ -129,6 +132,12 @@
 /// Always use text-based completion.
 NeverParse,
   } RunParser = ParseIfReady;
+
+  /// Allows capturing various internal structures used by clangd during code
+  /// completion. Eg: Symbol quality and relevance signals of all the candidates
+  /// in the output.
+  std::function *RecordCCResult = nullptr;
 };
 
 // Semi-structured representation of a code-complete suggestion for our C++ API.
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -66,6 +66,7 @@
 #include "llvm/Support/ScopedPrinter.h"
 #include 
 #include 
+#include 
 
 // We log detailed candidate here if you run with -debug-only=codecomplete.
 #define DEBUG_TYPE "CodeComplete"
@@ -1660,6 +1661,9 @@
? Scores.Total / Relevance.NameMatch
: Scores.Quality;
 
+if (Opts.RecordCCResult)
+  (*Opts.RecordCCResult)(toCodeCompletion(Bundle), Quality, Relevance);
+
 dlog("CodeComplete: {0} ({1}) = {2}\n{3}{4}\n", First.Name,
  llvm::to_string(Origin), Scores.Total, llvm::to_string(Quality),
  llvm::to_string(Relevance));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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* 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 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 QualitySignals;
+std::shared_ptr 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 248240.
usaxena95 marked 2 inline comments as done.
usaxena95 added a comment.

Remove ununsed import.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75603

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeComplete.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp


Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -29,7 +29,9 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -1041,6 +1043,24 @@
   EXPECT_THAT(Completions, Contains(Named("TT")));
 }
 
+TEST(CompletionTest, EnableInstrumentationMode) {
+  std::vector RecordedCompletions;
+  std::function
+  ResultRecorder =
+  [&RecordedCompletions](const CodeCompletion &CC,
+ const SymbolQualitySignals &,
+ const SymbolRelevanceSignals &) -> void {
+RecordedCompletions.push_back(CC);
+  };
+  CodeCompleteOptions Opts;
+  Opts.RecordCCResult = &ResultRecorder;
+
+  completions("int xy1, xy2; int a = xy^", /*IndexSymbols=*/{}, Opts);
+  EXPECT_THAT(RecordedCompletions,
+  UnorderedElementsAre(Named("xy1"), Named("xy2")));
+}
+
 SignatureHelp signatures(llvm::StringRef Text, Position Point,
  std::vector IndexSymbols = {}) {
   std::unique_ptr Index;
Index: clang-tools-extra/clangd/CodeComplete.h
===
--- clang-tools-extra/clangd/CodeComplete.h
+++ clang-tools-extra/clangd/CodeComplete.h
@@ -19,6 +19,7 @@
 #include "Logger.h"
 #include "Path.h"
 #include "Protocol.h"
+#include "Quality.h"
 #include "index/Index.h"
 #include "index/Symbol.h"
 #include "index/SymbolOrigin.h"
@@ -29,12 +30,14 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
+#include 
 #include 
 
 namespace clang {
 class NamedDecl;
 namespace clangd {
 struct PreambleData;
+struct CodeCompletion;
 
 struct CodeCompleteOptions {
   /// Returns options that can be passed to clang's completion engine.
@@ -129,6 +132,12 @@
 /// Always use text-based completion.
 NeverParse,
   } RunParser = ParseIfReady;
+
+  /// Allows capturing various internal structures used by clangd during code
+  /// completion. Eg: Symbol quality and relevance signals of all the 
candidates
+  /// in the output.
+  std::function *RecordCCResult = 
nullptr;
 };
 
 // Semi-structured representation of a code-complete suggestion for our C++ 
API.
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -1660,6 +1660,9 @@
? Scores.Total / Relevance.NameMatch
: Scores.Quality;
 
+if (Opts.RecordCCResult)
+  (*Opts.RecordCCResult)(toCodeCompletion(Bundle), Quality, Relevance);
+
 dlog("CodeComplete: {0} ({1}) = {2}\n{3}{4}\n", First.Name,
  llvm::to_string(Origin), Scores.Total, llvm::to_string(Quality),
  llvm::to_string(Relevance));


Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -29,7 +29,9 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -1041,6 +1043,24 @@
   EXPECT_THAT(Completions, Contains(Named("TT")));
 }
 
+TEST(CompletionTest, EnableInstrumentationMode) {
+  std::vector RecordedCompletions;
+  std::function
+  ResultRecorder =
+  [&RecordedCompletions](const CodeCompletion &CC,
+ const SymbolQualitySignals &,
+ const SymbolRelevanceSignals &) -> void {
+RecordedCompletions.push_back(CC);
+  };
+  CodeCompleteOptions Opts;
+  Opts.RecordCCResult = &ResultRecorder;
+
+  completions("int xy1, xy2; int a = xy^", /*IndexSymbols=*/{}, Opts);
+  EXPECT_THAT(RecordedCompletions,
+  UnorderedElementsAre(Named("xy1"), Named("xy2")));
+}
+
 SignatureHelp signatures(llvm::StringRef Text, Position Point,
  std::vector IndexSymbols = {}) {
   std::unique_ptr Index;
Index: clang-tools-extra/clangd/CodeComplete.h
===
--- clang-tools-extra/clangd/CodeComplete.h
+++ clang-tools-extra/clangd/CodeComplete.h
@@ -19,6 +19,

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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/CodeComplete.h:136
+
+  /// Allows capturing various internal structures used by clangd during code
+  /// completion. Eg: Symbol quality and relevance signals of all the 
candidates

First say what the behavior/API is (called once for each result...), Then 
justify it :)



Comment at: clang-tools-extra/clangd/CodeComplete.h:139
+  /// in the output.
+  std::function *RecordCCResult = 
nullptr;

I'd suggest including the final score in the signature rather than recompute 
it, just so the contract is really clear and simple (results yielded in 
arbitrary order, will be ranked by -score). Please spell this out.



Comment at: clang-tools-extra/clangd/CodeComplete.h:140
+  std::function *RecordCCResult = 
nullptr;
 };

This doesn't need to be a pointer, std::function is copy/movable and supports 
nullptr as a sentinel value.



Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1048
+  std::vector RecordedCompletions;
+  std::function

Nit: typically `auto` here (the anonymous lambda type) and let it convert to 
function implicitly when needed.

No need for `-> type` in trivial cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75603



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Forgot to mention: I also think the trace approach certainly has things going 
for it, or even parsing out the messages from the existing logs.
But in this particular case the callback happens to be extremely convenient and 
also not invasive (since the data structures are already exposed, code complete 
has an opts struct etc). And since this is for analysis we have a lot of 
flexibility to rework if it stops being easy to maintain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75603



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D75603#1906418 , @sammccall wrote:

> Forgot to mention: I also think the trace approach certainly has things going 
> for it, or even parsing out the messages from the existing logs.
>  But in this particular case the callback happens to be extremely convenient 
> and also not invasive (since the data structures are already exposed, code 
> complete has an opts struct etc). And since this is for analysis we have a 
> lot of flexibility to rework if it stops being easy to maintain.


as discussed offline, I was rather afraid of the initial version of the patch, 
but the final version seems ok as it only adds a single field to 
codecompleteopts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75603



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 248416.
usaxena95 marked 5 inline comments as done.
usaxena95 added a comment.

Addressed comments.

- Populated score in CodeCompletion before invoking the callback.
- Tested that CodeCompletion is scored
- Updated comment for callback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75603

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeComplete.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -29,7 +29,9 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -69,6 +71,7 @@
 MATCHER(InsertInclude, "") {
   return !arg.Includes.empty() && bool(arg.Includes[0].Insertion);
 }
+MATCHER(Scored, "") { return arg.Score.Total > 0; }
 MATCHER_P(SnippetSuffix, Text, "") { return arg.SnippetSuffix == Text; }
 MATCHER_P(Origin, OriginSet, "") { return arg.Origin == OriginSet; }
 MATCHER_P(Signature, S, "") { return arg.Signature == S; }
@@ -1041,6 +1044,21 @@
   EXPECT_THAT(Completions, Contains(Named("TT")));
 }
 
+TEST(CompletionTest, EnableInstrumentationMode) {
+  std::vector RecordedCompletions;
+  CodeCompleteOptions Opts;
+  Opts.RecordCCResult = [&RecordedCompletions](const CodeCompletion &CC,
+   const SymbolQualitySignals &,
+   const SymbolRelevanceSignals &) {
+RecordedCompletions.push_back(CC);
+  };
+
+  completions("int xy1, xy2; int a = xy^", /*IndexSymbols=*/{}, Opts);
+  EXPECT_THAT(RecordedCompletions,
+  UnorderedElementsAre(AllOf(Named("xy1"), Scored()),
+   AllOf(Named("xy2"), Scored(;
+}
+
 SignatureHelp signatures(llvm::StringRef Text, Position Point,
  std::vector IndexSymbols = {}) {
   std::unique_ptr Index;
Index: clang-tools-extra/clangd/CodeComplete.h
===
--- clang-tools-extra/clangd/CodeComplete.h
+++ clang-tools-extra/clangd/CodeComplete.h
@@ -19,6 +19,7 @@
 #include "Logger.h"
 #include "Path.h"
 #include "Protocol.h"
+#include "Quality.h"
 #include "index/Index.h"
 #include "index/Symbol.h"
 #include "index/SymbolOrigin.h"
@@ -29,12 +30,14 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
+#include 
 #include 
 
 namespace clang {
 class NamedDecl;
 namespace clangd {
 struct PreambleData;
+struct CodeCompletion;
 
 struct CodeCompleteOptions {
   /// Returns options that can be passed to clang's completion engine.
@@ -129,6 +132,16 @@
 /// Always use text-based completion.
 NeverParse,
   } RunParser = ParseIfReady;
+
+  /// Callback invoked on all CompletionCandidate after they are scored and
+  /// before they are ranked (by -Score). Thus the results are yielded in
+  /// arbitrary order. CodeCompletion also contains the computed Score.
+  ///
+  /// This callbacks allows capturing various internal structures used by clangd
+  /// during code completion. Eg: Symbol quality and relevance signals.
+  std::function
+  RecordCCResult;
 };
 
 // Semi-structured representation of a code-complete suggestion for our C++ API.
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -1660,6 +1660,13 @@
? Scores.Total / Relevance.NameMatch
: Scores.Quality;
 
+if (Opts.RecordCCResult) {
+  CodeCompletion Completion = toCodeCompletion(Bundle);
+  Completion.Score = Scores;
+  Completion.CompletionTokenRange = ReplacedRange;
+  Opts.RecordCCResult(Completion, Quality, Relevance);
+}
+
 dlog("CodeComplete: {0} ({1}) = {2}\n{3}{4}\n", First.Name,
  llvm::to_string(Origin), Scores.Total, llvm::to_string(Quality),
  llvm::to_string(Relevance));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 added inline comments.



Comment at: clang-tools-extra/clangd/CodeComplete.h:139
+  /// in the output.
+  std::function *RecordCCResult = 
nullptr;

sammccall wrote:
> I'd suggest including the final score in the signature rather than recompute 
> it, just so the contract is really clear and simple (results yielded in 
> arbitrary order, will be ranked by -score). Please spell this out.
Couldn't add it to the signature since inner classes cannot be forward declared.
Since `CodeCompletion` contains the `Score`, I have populated this field in the 
`CodeCompletion` (and also `CompletionTokenRange`) as done in 
`toCodeCompleteResult` to be consistent.

Also tested that the CodeCompletion is scored.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75603



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 248422.
usaxena95 added a comment.

Passed score as a float as an explicit argument of the callback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75603

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeComplete.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp


Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -29,7 +29,9 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -1041,6 +1043,21 @@
   EXPECT_THAT(Completions, Contains(Named("TT")));
 }
 
+TEST(CompletionTest, RecordCCResultCallback) {
+  std::vector RecordedCompletions;
+  CodeCompleteOptions Opts;
+  Opts.RecordCCResult = [&RecordedCompletions](const CodeCompletion &CC,
+   const SymbolQualitySignals &,
+   const SymbolRelevanceSignals &,
+   float Score) {
+RecordedCompletions.push_back(CC);
+  };
+
+  completions("int xy1, xy2; int a = xy^", /*IndexSymbols=*/{}, Opts);
+  EXPECT_THAT(RecordedCompletions,
+  UnorderedElementsAre(Named("xy1"), Named("xy2")));
+}
+
 SignatureHelp signatures(llvm::StringRef Text, Position Point,
  std::vector IndexSymbols = {}) {
   std::unique_ptr Index;
Index: clang-tools-extra/clangd/CodeComplete.h
===
--- clang-tools-extra/clangd/CodeComplete.h
+++ clang-tools-extra/clangd/CodeComplete.h
@@ -19,6 +19,7 @@
 #include "Logger.h"
 #include "Path.h"
 #include "Protocol.h"
+#include "Quality.h"
 #include "index/Index.h"
 #include "index/Symbol.h"
 #include "index/SymbolOrigin.h"
@@ -29,12 +30,14 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
+#include 
 #include 
 
 namespace clang {
 class NamedDecl;
 namespace clangd {
 struct PreambleData;
+struct CodeCompletion;
 
 struct CodeCompleteOptions {
   /// Returns options that can be passed to clang's completion engine.
@@ -129,6 +132,16 @@
 /// Always use text-based completion.
 NeverParse,
   } RunParser = ParseIfReady;
+
+  /// Callback invoked on all CompletionCandidate after they are scored and
+  /// before they are ranked (by -Score). Thus the results are yielded in
+  /// arbitrary order.
+  ///
+  /// This callbacks allows capturing various internal structures used by 
clangd
+  /// during code completion. Eg: Symbol quality and relevance signals.
+  std::function
+  RecordCCResult;
 };
 
 // Semi-structured representation of a code-complete suggestion for our C++ 
API.
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -1660,6 +1660,10 @@
? Scores.Total / Relevance.NameMatch
: Scores.Quality;
 
+if (Opts.RecordCCResult)
+  Opts.RecordCCResult(toCodeCompletion(Bundle), Quality, Relevance,
+  Scores.Total);
+
 dlog("CodeComplete: {0} ({1}) = {2}\n{3}{4}\n", First.Name,
  llvm::to_string(Origin), Scores.Total, llvm::to_string(Quality),
  llvm::to_string(Relevance));


Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -29,7 +29,9 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -1041,6 +1043,21 @@
   EXPECT_THAT(Completions, Contains(Named("TT")));
 }
 
+TEST(CompletionTest, RecordCCResultCallback) {
+  std::vector RecordedCompletions;
+  CodeCompleteOptions Opts;
+  Opts.RecordCCResult = [&RecordedCompletions](const CodeCompletion &CC,
+   const SymbolQualitySignals &,
+   const SymbolRelevanceSignals &,
+   float Score) {
+RecordedCompletions.push_back(CC);
+  };
+
+  completions("int xy1, xy2; int a = xy^", /*IndexSymbols=*/{}, Opts);
+  EXPECT_THAT(RecordedCompletions,
+  UnorderedElementsAre(Named("xy1"), Named("xy2")));
+}
+
 SignatureHelp signatures(llvm::StringRef Text, Position Point,
  std::vector IndexSymbols = {}) {
   std::unique_ptr Index;
Index: clan

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

[UTKARSH SAXENA via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe397a0a5c3c0: [clangd] Add instrumentation mode in clangd 
for metrics collection. (authored by usaxena95).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75603

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/CodeComplete.h
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp


Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -29,7 +29,9 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -1041,6 +1043,21 @@
   EXPECT_THAT(Completions, Contains(Named("TT")));
 }
 
+TEST(CompletionTest, RecordCCResultCallback) {
+  std::vector RecordedCompletions;
+  CodeCompleteOptions Opts;
+  Opts.RecordCCResult = [&RecordedCompletions](const CodeCompletion &CC,
+   const SymbolQualitySignals &,
+   const SymbolRelevanceSignals &,
+   float Score) {
+RecordedCompletions.push_back(CC);
+  };
+
+  completions("int xy1, xy2; int a = xy^", /*IndexSymbols=*/{}, Opts);
+  EXPECT_THAT(RecordedCompletions,
+  UnorderedElementsAre(Named("xy1"), Named("xy2")));
+}
+
 SignatureHelp signatures(llvm::StringRef Text, Position Point,
  std::vector IndexSymbols = {}) {
   std::unique_ptr Index;
Index: clang-tools-extra/clangd/CodeComplete.h
===
--- clang-tools-extra/clangd/CodeComplete.h
+++ clang-tools-extra/clangd/CodeComplete.h
@@ -19,6 +19,7 @@
 #include "Logger.h"
 #include "Path.h"
 #include "Protocol.h"
+#include "Quality.h"
 #include "index/Index.h"
 #include "index/Symbol.h"
 #include "index/SymbolOrigin.h"
@@ -29,12 +30,14 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
+#include 
 #include 
 
 namespace clang {
 class NamedDecl;
 namespace clangd {
 struct PreambleData;
+struct CodeCompletion;
 
 struct CodeCompleteOptions {
   /// Returns options that can be passed to clang's completion engine.
@@ -129,6 +132,16 @@
 /// Always use text-based completion.
 NeverParse,
   } RunParser = ParseIfReady;
+
+  /// Callback invoked on all CompletionCandidate after they are scored and
+  /// before they are ranked (by -Score). Thus the results are yielded in
+  /// arbitrary order.
+  ///
+  /// This callbacks allows capturing various internal structures used by 
clangd
+  /// during code completion. Eg: Symbol quality and relevance signals.
+  std::function
+  RecordCCResult;
 };
 
 // Semi-structured representation of a code-complete suggestion for our C++ 
API.
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -1660,6 +1660,10 @@
? Scores.Total / Relevance.NameMatch
: Scores.Quality;
 
+if (Opts.RecordCCResult)
+  Opts.RecordCCResult(toCodeCompletion(Bundle), Quality, Relevance,
+  Scores.Total);
+
 dlog("CodeComplete: {0} ({1}) = {2}\n{3}{4}\n", First.Name,
  llvm::to_string(Origin), Scores.Total, llvm::to_string(Quality),
  llvm::to_string(Relevance));


Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -29,7 +29,9 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -1041,6 +1043,21 @@
   EXPECT_THAT(Completions, Contains(Named("TT")));
 }
 
+TEST(CompletionTest, RecordCCResultCallback) {
+  std::vector RecordedCompletions;
+  CodeCompleteOptions Opts;
+  Opts.RecordCCResult = [&RecordedCompletions](const CodeCompletion &CC,
+   const SymbolQualitySignals &,
+   const SymbolRelevanceSignals &,
+   float Score) {
+RecordedCompletions.push_back(CC);
+  };
+
+  completions("int xy1, xy2; int a = xy^", /*IndexSymbols=*/{}, Opts);
+  EXPECT_THAT(RecordedCompletions,
+  UnorderedElementsAre(Named("xy1"), Named("xy2")));
+}
+
 SignatureHelp signatures(llvm::StringRef Text, Position Point,
  std::vector