[PATCH] D42480: [clangd] Provide a helper to report estimated memory usage per-file

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.h:324
 
+  /// Returns estimated memory usage for each of the currently files.
+  /// The order of results is unspecified.
----------------
ioeric wrote:
> s/currently/current/ ? or current open?
Intended this to be `currently open` files. Fixed, thanks!


================
Comment at: clangd/ClangdServer.h:326
+  /// The order of results is unspecified.
+  std::vector<std::pair<Path, std::size_t>> getUsedBytesPerFile() const;
+
----------------
ioeric wrote:
> Is the total usage of all files a good estimate of the overall clangd memory usage? It might be worth mentioning the relationship between this and overall clangd memory here.
It isn't.
Added a comment.


================
Comment at: clangd/ClangdUnit.cpp:664
+  std::lock_guard<std::mutex> Lock(Mutex);
+  return ASTMemUsage + PreambleMemUsage;
+}
----------------
ioeric wrote:
> Would it be possible to calculate the usages lazily on request, so that you don't need to maintain two states?
It's really complicated since we store `std::future`s. This is one of the caveats that I'm trying to fix with threading revamp, implementation will change accordingly. For now, having two states is much easier.


================
Comment at: clangd/ClangdUnit.h:100
+  /// Returns the esitmated size of the AST and the accessory structures, in
+  /// bytes. Does not include the size of the preamble.
+  std::size_t getUsedBytes() const;
----------------
ioeric wrote:
> It seems that `CppFile::getUsedBytes()` includes preamble?
`CppFile` includes the size of the `Preamble`, `ParsedAST` doesn't


================
Comment at: unittests/clangd/ClangdTests.cpp:428
 
+TEST_F(ClangdVFSTest, MemoryUsage) {
+  MockFSProvider FS;
----------------
ioeric wrote:
> Can we add a test for non-empty files and check that the usage is reasonable (e.g. `>0`)?
We do exactly that. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42480