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