This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex
ClosedPublic

Authored by kadircet on Sep 28 2020, 6:18 AM.

Details

Summary

File-granular information is considered details.

Depends on D88411

Diff Detail

Event Timeline

kadircet created this revision.Sep 28 2020, 6:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2020, 6:18 AM
kadircet requested review of this revision.Sep 28 2020, 6:18 AM
sammccall added inline comments.Oct 5 2020, 3:01 AM
clang-tools-extra/clangd/index/Background.cpp
420

Hmm, we should be careful now, estimateMemoryUsage() is "total" while addUsage takes "self".

We're not overriding estimateMemoryUsage here to include IndexedSymbols, but this is probably just an oversight. Consider calling SwapIndex::estimateMemoryUsage() explicitly to guard/communicate against this.

In the long-run, we should put attachMemoryUsage on SymbolIndex and either remove estimateMemoryUsage() or make it non-virtual (MemoryTree R; attachMemoryUsage(R); return R.total())

clang-tools-extra/clangd/index/Background.h
177

As a conventional name for these, profile() or so might be slightly more evocative. And I think we should push this into progressively more places (that currently have ad-hoc "estimate" accessors) so the brevity is reasonable I think.

(If not, I'd even slightly prefer "record"/"measure" over "attach" as emphasizing the high-level operation rather than the data structure used can help readability)

clang-tools-extra/clangd/index/FileIndex.cpp
395

addChild("symbols"/"refs"/"relations")?
This seems like really useful information

481

addChild("preamble").addChild("symbols")?

kadircet updated this revision to Diff 296684.Oct 7 2020, 8:17 AM
kadircet marked 4 inline comments as done.
  • Rename attachMemoryUsage to profile
  • Split file symbols into symbols/refs/relations granularity
  • Rebase
sammccall accepted this revision.Oct 7 2020, 9:51 AM

Now there's lots of usage (which looks good!) i'm finding it a bit hard to keep track of what the tree will look like overall.

At some point it'd be great to:
a) bind this to an LSP extension so we can see it in editors
b) add a lit test that calls that extension method and includes a dump of the output

This revision is now accepted and ready to land.Oct 7 2020, 9:51 AM

Now there's lots of usage (which looks good!) i'm finding it a bit hard to keep track of what the tree will look like overall.

At some point it'd be great to:
a) bind this to an LSP extension so we can see it in editors

i was also thinking about it and couldn't decide between a "custom command" vs "code action".

  • the former gives a richer interaction, but requires every editor plugin to implement support.
  • the latter is a little bit more restrictive but doesn't require a bunch of extra work.

I am happy to go with the "code action" approach initially. WDYT? (not in the scope of this patch)

kadircet updated this revision to Diff 296971.Oct 8 2020, 7:32 AM
  • Fix a bug in FileSymbols profiling.
  • Add unittests to ensure tree structure is as expected.

Now there's lots of usage (which looks good!) i'm finding it a bit hard to keep track of what the tree will look like overall.

At some point it'd be great to:
a) bind this to an LSP extension so we can see it in editors

i was also thinking about it and couldn't decide between a "custom command" vs "code action".

  • the former gives a richer interaction, but requires every editor plugin to implement support.
  • the latter is a little bit more restrictive but doesn't require a bunch of extra work.

I am happy to go with the "code action" approach initially. WDYT? (not in the scope of this patch)

I'm pretty leery about code action because it's not at all context-sensitive (not even per-file).
That also doesn't give us any way to specify detail vs summary if we want that.
I'd suggest a custom LSP method (unless there's some reason to prefer executeCommand? Seems like additional indirection for no benefit).

Now there's lots of usage (which looks good!) i'm finding it a bit hard to keep track of what the tree will look like overall.

At some point it'd be great to:
a) bind this to an LSP extension so we can see it in editors

i was also thinking about it and couldn't decide between a "custom command" vs "code action".

  • the former gives a richer interaction, but requires every editor plugin to implement support.
  • the latter is a little bit more restrictive but doesn't require a bunch of extra work.

I am happy to go with the "code action" approach initially. WDYT? (not in the scope of this patch)

I'm pretty leery about code action because it's not at all context-sensitive (not even per-file).

Another slighty silly reason: because of layering, a code action is going to have a hard time getting at ClangdLSPServer's profile (or even ClangdServer's).
Whereas a custom method will be implemented at that layer.

(We could work around this in various ways, but I think we'll create a bit of a mess)

This revision was landed with ongoing or failed builds.Oct 12 2020, 6:27 AM
This revision was automatically updated to reflect the committed changes.