[clangd] Use JSON format in benchmark requests reader

[kbobyrev]

Summary

After FuzzyFindRequest JSON (de)serialization was introduced, it should replace ad-hoc fuzzy-find request parsing implemented in the IndexBenchmark driver.

[ilya-biryukov]

clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp

21

We don't need the usings, just shorten the name on usage sites (usages are inside namespace clangd)

36–38

Maybe read a JSON array from the file instead?
I.e. one would have to write [{/*request1*/}, {/*request2*/}, ..] instead of putting a request per line

Pros: we get valid json, can have prettified forms too.
Cons: might be a bit harder to construct the json array. But don't think it's a big deal.

[kbobyrev]

Apparently, the parsing seems to be wrong; I should fix that first.

[ilya-biryukov]

In D51971#1232064, @kbobyrev wrote:

Apparently, the parsing seems to be wrong; I should fix that first.

Oh, yeah, just use llvm::json::parse

[kbobyrev]

@ilya-biryukov thanks! I was assuming that the StringRef constructor parses user-provided string :(

llvm::json::Array is used now.

[ilya-biryukov]

clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp

65

Return from function after error?

67

Maybe declare this var inside the loop where it's used?

68

Check that the parsed value is an array and report an error if not?

[kbobyrev]

clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp

65

I thought that this should panic instead of returning empty Requests: otherwise it wouldn't be possible to detect problems in the benchmark driver in test, for example (it will just run benchmark over empty list of requests instead of doing something useful).

[ilya-biryukov]

clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp

65

Let's "panic" explicitly, e.g. with exit(1).
We're currently triggering undefined behaviour, not something we want to do.

[kbobyrev]

clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp

65

Good point; fixed!

[kbobyrev]

Rebase on top of rL342123.

[kbobyrev]

Fix the diff after rebase.

[kbobyrev]

Fix empty scope in requests.json: %s/"::"/""/g.

[ilya-biryukov]

clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp

47

If there was a json parsing error, we should consume it and print to the user, i.e.

llvm::errs() << "Error when parsing json requests file: " << llvm::toString(std::move(JSONArray));

The user-facing error when an object is not an array should probably be a separate one.

64

Check for return value of fromJSON and also report an error if it failed?

[ilya-biryukov]

clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp

48

We should only call takeError() when JSONArray is an error. Not an array value should be handled separately.

BTW, can we add tests with invalid inputs for both of these cases?

60

Maybe also output the request that failed to parse?
To help untangle the problem in case it happens.

[kbobyrev]

Fix a typo in test comments.

[ilya-biryukov]

LGTM

clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp

52

NIT: technically, the parsing succeeded at this point, maybe change to something like "top-level value is not a json array"?

60

NIT: s/parsing/deserializing?

clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp

 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include <fstream>
 #include <streambuf>
 #include <string>
 
 const char *IndexFilename;
-const char *LogFilename;
+const char *RequestsFilename;
 
 namespace clang {
 namespace clangd {
 namespace {
 
-std::unique_ptr<clang::clangd::SymbolIndex> buildMem() {
-  return clang::clangd::loadIndex(IndexFilename, {}, false);
+std::unique_ptr<SymbolIndex> buildMem() {
+  return loadIndex(IndexFilename, {}, false);
 }
 
-std::unique_ptr<clang::clangd::SymbolIndex> buildDex() {
-  return clang::clangd::loadIndex(IndexFilename, {}, true);
+std::unique_ptr<SymbolIndex> buildDex() {
+  return loadIndex(IndexFilename, {}, true);
 }
 
-// This function processes user-provided Log file with fuzzy find requests in
-// the following format:
-//
-// fuzzyFind("UnqualifiedName", scopes=["clang::", "clang::clangd::"])
-//
-// It constructs vector of FuzzyFindRequests which is later used for the
-// benchmarks.
-std::vector<clang::clangd::FuzzyFindRequest> extractQueriesFromLogs() {
-  llvm::Regex RequestMatcher("fuzzyFind\\(\"([a-zA-Z]*)\", scopes=\\[(.*)\\]");
-  llvm::SmallVector<llvm::StringRef, 200> Matches;
-  std::ifstream InputStream(LogFilename);
+// Reads JSON array of serialized FuzzyFindRequest's from user-provided file.
+std::vector<FuzzyFindRequest> extractQueriesFromLogs() {
+  std::ifstream InputStream(RequestsFilename);
   std::string Log((std::istreambuf_iterator<char>(InputStream)),
                   std::istreambuf_iterator<char>());
-  llvm::StringRef Temporary(Log);
-  llvm::SmallVector<llvm::StringRef, 200> Strings;
-  Temporary.split(Strings, '\n');
 
-  clang::clangd::FuzzyFindRequest R;
-  R.MaxCandidateCount = 100;
+  std::vector<FuzzyFindRequest> Requests;
+  auto JSONArray = llvm::json::parse(Log);
 
-  llvm::SmallVector<llvm::StringRef, 200> CommaSeparatedValues;
-
-  std::vector<clang::clangd::FuzzyFindRequest> RealRequests;
-  for (auto Line : Strings) {
-    if (RequestMatcher.match(Line, &Matches)) {
-      R.Query = Matches[1];
-      CommaSeparatedValues.clear();
-      Line.split(CommaSeparatedValues, ',');
-      R.Scopes.clear();
-      for (auto C : CommaSeparatedValues) {
-        R.Scopes.push_back(C);
-      }
-      RealRequests.push_back(R);
+  // Panic if the provided file couldn't be parsed.
+  if (!JSONArray) {
+    llvm::errs() << "Error when parsing JSON requests file: "
+                 << llvm::toString(JSONArray.takeError());
+    exit(1);
+  }
+  if (!JSONArray->getAsArray()) {
+    llvm::errs() << "Error when parsing JSON array: " << Log << '\n';
+    exit(1);
+  }
+
+  for (const auto &Item : *JSONArray->getAsArray()) {
+    FuzzyFindRequest Request;
+    // Panic if the provided file couldn't be parsed.
+    if (!fromJSON(Item, Request)) {
+      llvm::errs() << "Error when parsing request: " << Item << '\n';
+      exit(1);
     }
+    Requests.push_back(Request);
   }
-  return RealRequests;
+  return Requests;
 }
 
 static void MemQueries(benchmark::State &State) {
   const auto Mem = buildMem();
   const auto Requests = extractQueriesFromLogs();
   for (auto _ : State)
     for (const auto &Request : Requests)
       Mem->fuzzyFind(Request, [](const Symbol &S) {});
 }
 BENCHMARK(MemQueries);
 

 
 // FIXME(kbobyrev): Add index building time benchmarks.
 // FIXME(kbobyrev): Add memory consumption "benchmarks" by manually measuring
 // in-memory index size and reporting it as time.
 // FIXME(kbobyrev): Create a logger wrapper to suppress debugging info printer.
 int main(int argc, char *argv[]) {
   if (argc < 3) {
     llvm::errs() << "Usage: " << argv[0]
-                 << " global-symbol-index.yaml fuzzy-find-requests.log "
+                 << " global-symbol-index.yaml requests.json "
                     "BENCHMARK_OPTIONS...\n";
     return -1;
   }
   IndexFilename = argv[1];
-  LogFilename = argv[2];
+  RequestsFilename = argv[2];
   // Trim first two arguments of the benchmark invocation.
   argv += 3;
   argc -= 3;
   ::benchmark::Initialize(&argc, argv);
   ::benchmark::RunSpecifiedBenchmarks();
 }

clang-tools-extra/test/clangd/Inputs/requests.json

+[{"MaxCandidateCount":100,"ProximityPaths":["/usr/home/user/clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp"],"Query":"OMP","RestrictForCodeCompletion":true,"Scopes":["clang::"]},
+{"MaxCandidateCount":100,"ProximityPaths":[],"Query":"s","RestrictForCodeCompletion":true,"Scopes":["llvm::", ""]},
+{"MaxCandidateCount":100,"ProximityPaths":[],"Query":"sy","RestrictForCodeCompletion":true,"Scopes":["llvm::", ""]},
+{"MaxCandidateCount":100,"ProximityPaths":[],"Query":"sys","RestrictForCodeCompletion":true,"Scopes":["llvm::", ""]},
+{"MaxCandidateCount":100,"ProximityPaths":[],"Query":"sys","RestrictForCodeCompletion":true,"Scopes":["llvm::", ""]},
+{"MaxCandidateCount":100,"ProximityPaths":[],"Query":"Dex","RestrictForCodeCompletion":true,"Scopes":["clang::clangd::", "clang::", "clang::clangd::dex::"]},
+{"MaxCandidateCount":100,"ProximityPaths":[],"Query":"Variable","RestrictForCodeCompletion":true,"Scopes":[""]}]

clang-tools-extra/test/clangd/Inputs/requests.log

-V[09:08:34.301] Code complete: fuzzyFind("s", scopes=[llvm::])
-V[09:08:34.368] Code complete: fuzzyFind("sy", scopes=[llvm::])
-V[09:08:34.442] Code complete: fuzzyFind("sys", scopes=[llvm::])
-V[09:09:12.075] Code complete: fuzzyFind("Dex", scopes=[clang::clangd::, clang::, clang::clangd::dex::])
-V[09:09:12.075] Code complete: fuzzyFind("Variable", scopes=[])

clang-tools-extra/test/clangd/index-tools.test

 # RUN: clangd-indexer %p/Inputs/BenchmarkSource.cpp -- -I%p/Inputs > %t.index
 # FIXME: By default, benchmarks are excluded from the list of default targets hence not built. Find a way to depend on benchmarks to run the next command.
-# RUN: if [ -f %clangd-benchmark-dir/IndexBenchmark ]; then %clangd-benchmark-dir/IndexBenchmark %t.index %p/Inputs/requests.log --benchmark_min_time=0.01 ; fi
+# RUN: if [ -f %clangd-benchmark-dir/IndexBenchmark ]; then %clangd-benchmark-dir/IndexBenchmark %t.index %p/Inputs/requests.json --benchmark_min_time=0.01 ; fi
+# Pass invalid JSON file and check that benchmarks fails to parse it.
+# RUN: if [ -f %clangd-benchmark-dir/IndexBenchmark ]; then not %clangd-benchmark-dir/IndexBenchmark %t.index %t --benchmark_min_time=0.01 ; fi