Page MenuHomePhabricator

[clangd] Add a way for exporting memory usage metrics
Changes PlannedPublic

Authored by kadircet on Aug 17 2020, 7:20 AM.

Details

Reviewers
sammccall
adamcz
Summary

Introduces a helper to collect memory usage metrics from multiple
components.
Adds measurements for dynamic index, ast cache and preambles.

Diff Detail

Event Timeline

kadircet created this revision.Aug 17 2020, 7:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2020, 7:20 AM
kadircet requested review of this revision.Aug 17 2020, 7:20 AM
adamcz added inline comments.Aug 17 2020, 7:46 AM
clang-tools-extra/clangd/TUScheduler.cpp
172

Any idea how expensive this is? I suppose TUScheduler::update() is rare enough that it's not a big deal?

I ask because of bad experience with estimating memory usage inside critical path on another project ;-)

Maybe we can add a span around this, so we can see if it's expensive in traces? Or maybe that will turn out to be more expensive than memory estimation?

1300

Isn't FD->Worker->update() above async (with correct options)? If so, this is still reading the old data, right?

1312

Would it make sense to collect the number of preambles and ASTs cached at this point as well? Exported under a different metric, but at the same time, to be able to join the data together?

kadircet added inline comments.Aug 17 2020, 8:07 AM
clang-tools-extra/clangd/TUScheduler.cpp
172

TUScheduler::update() is rare enough

actually it can potentially be triggered after every keystroke. I suppose it might make sense to move this out of the main thread completely. I suppose runWithPreambles action might be a better place to record, which is still quite often btw (every signature help and code completion request goes through it), but runs on a separate thread.

i would expect that will turn out to be more expensive than memory estimation to be the case, will do some checks though.

Most of the calculations happening in there is just addition of some members. the source manager makes me afraid a little bit tho, as it seems to be going over all the buffers.

1300

yes that's true. i was aiming for this to be an estimate, rather than an accurate view.

1312

yeah that definitely makes sense, but I would rather do that on a separate patch.

adamcz added inline comments.Aug 17 2020, 8:23 AM
clang-tools-extra/clangd/TUScheduler.cpp
168

nit: This is the same name as getUsedBytes(Key K). Maybe rename to getTotalUsedBytes()?

172

+1 to moving call to this to runWithPreamble or something like that. If a request gets cancelled, for example, there's no need to update the memory usage.

1300

I think this deserves a comment.

1310

So technically this is incorrect.
IdleASTs might contain AST for a file that is no longer tracked (e.g. had removeDocument() called on it). ASTCacheBytes will include it, but PreambleBytes will not, since it only asks for size of the tracked files. It's probably fine, considering how rare this will be and that these are all estimates, but I would recommend a comment, in case someone sees PreambleBytes be negative and wonders what's going on.

1312

Sounds good.

kadircet marked 8 inline comments as done.Aug 17 2020, 9:17 AM
kadircet added inline comments.
clang-tools-extra/clangd/TUScheduler.cpp
1310

right, thanks for pointing that out. added a comment, and clamped for non-negativity.

kadircet updated this revision to Diff 286043.Aug 17 2020, 9:17 AM
kadircet marked an inline comment as done.
  • Rename the overload
  • Add comments around possible caveats that might result in inaccuracies.
  • Move the metric recording itself into another thread.
  • Keep the calculations in the main thread, as they seemed to have <1ms latency, even with huge preambles/asts.
adamcz accepted this revision.Aug 17 2020, 9:20 AM
This revision is now accepted and ready to land.Aug 17 2020, 9:20 AM

Just for the record, as I think Kadir and Adam are both aware...

We discussed making this a bit richer and less reliant on static state.
We'd build a tree-shaped profile by passing a tree-builder recursively into various components.
Then the metrics would be exported at the top level, but we'd also want to expose it via debugging actions. (The tree edges from e.g. dynamic index to individual files would probably be optional - collapsed for metrics but present for debugging)

LMK if I have this wrong and you want to move forward with a simpler approch.

kadircet planned changes to this revision.Mon, Sep 21, 1:46 AM

Just for the record, as I think Kadir and Adam are both aware...

We discussed making this a bit richer and less reliant on static state.
We'd build a tree-shaped profile by passing a tree-builder recursively into various components.
Then the metrics would be exported at the top level, but we'd also want to expose it via debugging actions. (The tree edges from e.g. dynamic index to individual files would probably be optional - collapsed for metrics but present for debugging)

LMK if I have this wrong and you want to move forward with a simpler approch.

No that aligns perfectly with what I have in my mind as well. I just didn't have time to get back to this yet. Thanks a lot for summarizing offline discussions here!