[PATCH] D52047: [clangd] Add building benchmark and memory consumption tracking

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Also not sure about the trick:

- Would be surprising to see the "ms" instead of "mbytes"
- Time tends to vary between runs, so using Google Benchmark's capabilities to run multiple times and report aggregated times seems useful. Not so much for the memory usage: it's deterministic and running multiple times is just a waste of time.

I wonder if a separate (and trivial) C++ binary that would load the index and report the index size is any worse than the benchmark hack? What are the pros of the latter?



================
Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:81
+// Same for the next "benchmark".
+// FIXME(kbobyrev): Should this be separated into the BackingMemorySize
+// (underlying SymbolSlab size) and Symbol Index (MemIndex/Dex) overhead?
----------------
Given the trick we use for display, how are we going to show **two** memory uses?


================
Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:154
   ::benchmark::RunSpecifiedBenchmarks();
+  return 0;
 }
----------------
Since `return 0` is implied for main in C++, there's no need to add one.
May add clarity though, so feel free to keep it!



================
Comment at: clang-tools-extra/clangd/index/SymbolYAML.cpp:189
+  if (!Buffer) {
+    llvm::errs() << "Can't open " << SymbolFilename << "\n";
+    return llvm::None;
----------------
Return `llvm::Expected` instead of `Optional` and create errors with the specified text instead.
This is a slightly better option for the library code (callers can report errors in various ways, e.g. one can imagine reporting it in the LSP instead of stderr)


================
Comment at: clang-tools-extra/clangd/index/SymbolYAML.cpp:200
+    else
+      llvm::errs() << "Bad RIFF: " << llvm::toString(RIFF.takeError()) << "\n";
+  } else {
----------------
Same here: just propagate the error to the caller.


================
Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:239
   for (const auto &P : InvertedIndex)
-    Bytes += P.second.bytes();
+    Bytes += P.first.Data.size() + P.second.bytes();
   return Bytes + BackingDataSize;
----------------
Just to clarify: `P.first.Data.size()` is the size of the arena for all the symbols?


https://reviews.llvm.org/D52047