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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
