This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Provide a helper to report estimated memory usage per-file
ClosedPublic

Authored by ilya-biryukov on Jan 24 2018, 7:26 AM.

Diff Detail

Event Timeline

ilya-biryukov created this revision.Jan 24 2018, 7:26 AM
ioeric added inline comments.Jan 25 2018, 1:43 AM
clangd/ClangdServer.h
325

s/currently/current/ ? or current open?

327

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.

clangd/ClangdUnit.cpp
658

Would it be possible to calculate the usages lazily on request, so that you don't need to maintain two states?

clangd/ClangdUnit.h
100

It seems that CppFile::getUsedBytes() includes preamble?

unittests/clangd/ClangdTests.cpp
428

Can we add a test for non-empty files and check that the usage is reasonable (e.g. >0)?

ilya-biryukov marked 2 inline comments as done.Jan 25 2018, 4:16 AM
ilya-biryukov added inline comments.
clangd/ClangdServer.h
325

Intended this to be currently open files. Fixed, thanks!

327

It isn't.
Added a comment.

clangd/ClangdUnit.cpp
658

It's really complicated since we store std::futures. 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.

clangd/ClangdUnit.h
100

CppFile includes the size of the Preamble, ParsedAST doesn't

unittests/clangd/ClangdTests.cpp
428

We do exactly that.

ilya-biryukov marked 2 inline comments as done.
  • Fixed review comments
ioeric accepted this revision.Jan 25 2018, 4:23 AM

lg

clangd/ClangdUnit.cpp
658

Thanks for the explanation! A FIXME would be appropriate here then? In general, I wouldn't expect this API to be called very often.

unittests/clangd/ClangdTests.cpp
428

My bad!

struct Something {
  int method();
};

looks like real code on phabricator!

This revision is now accepted and ready to land.Jan 25 2018, 4:23 AM
ilya-biryukov marked 6 inline comments as done.
  • Added a FIXME to CppFile::getUsedBytes()

Thanks for reviewing this!

unittests/clangd/ClangdTests.cpp
428

Phabricator is too smart :-)

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.