[PATCH] D78429: [clangd] Metric tracking through Tracer

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Thanks!

In D78429#2013635 <https://reviews.llvm.org/D78429#2013635>, @kadircet wrote:

> Also while writing the test for rename I've noticed we were actually counting renamed files rather than occurrences, had to put a loop that would go over each file, PTAL.


Oh, I'd forgotten this. This was actually deliberate IIRC, I think the idea was much of the "cost" of a large rename grows with files rather than occurrences, so the limit of 50 applies to files.
I'd probably leave as-is and rename the metric to rename_files, but up to you.



================
Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:159
+  llvm::consumeError(Client.call(MethodName, {}).take().takeError());
+  EXPECT_THAT(Tracer.take("lsp_latency", MethodName), Not(testing::IsEmpty()));
+}
----------------
nit: ElementsAre(_) or SizeIs(1)?


================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:522
 
+  EXPECT_THAT(Tracer.take("ast_access_diag", "hit"), IsEmpty());
+  EXPECT_THAT(Tracer.take("ast_access_diag", "miss"), IsEmpty());
----------------
SizeIs(0)/SizeIs(1) might be more expressive here


================
Comment at: clang-tools-extra/clangd/unittests/TestTracer.h:35
+  /// Returns recorded measurements for \p Metric and clears them.
+  std::vector<double> take(std::string Metric, std::string Label = "");
+
----------------
ah, this should probably be takeMetric or so, because we might extend this class to cover non-metric tracing stuff.

(a slight misnomer, but "take measurement" collides with an english idiom and is also a bit unwieldy)


================
Comment at: clang-tools-extra/clangd/unittests/support/TestTracer.cpp:18
+                        llvm::StringRef Label) {
+  Measurements[Metric.Name][Label].push_back(Value);
+}
----------------
this needs a lock, the interface is documented as threadsafe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78429