[PATCH] D51090: [clangd] Add index benchmarks

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Nice and simple :-) Looks good, just some details.



================
Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:1
+//===--- DexBenchmark.cpp - DexIndex benchmarks -----------------*- C++ -*-===//
+//
----------------
rename? (it's not just dex now, right?)


================
Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:21
+
+std::string IndexFilename;
+std::string LogFilename;
----------------
const char* (no globals with nontrivial constructors/destructors)


================
Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:25
+
+std::unique_ptr<clang::clangd::SymbolIndex> buildMem() {
+  return clang::clangd::loadIndex(IndexFilename, {}, false);
----------------
these should be in ns clang::clangd::<anonymous> I think?


================
Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:26
+std::unique_ptr<clang::clangd::SymbolIndex> buildMem() {
+  return clang::clangd::loadIndex(IndexFilename, {}, false);
+}
----------------
This includes reading the file contents, which I'm not sure is a useful part of the BuildDex() benchmark.

You could consider splitting into loadIndex that takes content and loadIndexFromFile that takes a filename. The former would be more appropriate to time.


================
Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:33
+
+std::vector<clang::clangd::FuzzyFindRequest> extractQueriesFromLogs() {
+  llvm::Regex RequestMatcher("fuzzyFind\\(\"([a-zA-Z]*)\", scopes=\\[(.*)\\]");
----------------
comment?


================
Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:65
+std::vector<clang::clangd::FuzzyFindRequest> generateArtificialRequests() {
+  std::vector<clang::clangd::FuzzyFindRequest> Requests;
+  // FXIME(kbobyrev): Add more requests.
----------------
this seems a little weird - the extractQueriesFromLogs/buildDex seem to be agnostic to the index contents, but here we assume knowledge of it being LLVM.

The former model is nicer, and we can extend it by using the JSON representation to add more query attributes. Can we drop this function, and `ArtificialQueries->Queries`?


================
Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:104
+namespace clangd {
+namespace dex {
+
----------------
remove dex if this isn't dex-specific?


================
Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:132
+  for (auto _ : State)
+    buildDex();
+}
----------------
BTW this also counts *destruction* time. I think that's fine, though.


https://reviews.llvm.org/D51090