This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add $/dumpMemoryTree LSP extension
ClosedPublic

Authored by kadircet on Oct 12 2020, 3:13 PM.

Details

Summary

Performs a detailed profiling of clangd lsp server and conveys the
result to the client via showMessage.

Diff Detail

Event Timeline

kadircet created this revision.Oct 12 2020, 3:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2020, 3:13 PM
kadircet requested review of this revision.Oct 12 2020, 3:13 PM
nridge added a subscriber: nridge.Oct 12 2020, 4:42 PM

This is neat! Are there plans to add vscode client support to invoke this?

This is neat! Are there plans to add vscode client support to invoke this?

Yes, since we control the plugin in vscode we should have a way to invoke it there. (we already have support for switchHeaderSource).

We should first agree on the interaction though. This currently uses showMessage to display the dump, which is the most generic way LSP provides us with, but usually isn't good for multi-line output.
Other options we have are:

  • Write it into logs - Hard to find, but at least will look reasonable on every editor.
  • Insert as a commented block into the code. (like dumpast) - Clutters the code, but again looks reasonable on every editor and easier to find than logs.
  • Provide a json object in clangd and use fancy client side features like tree views. - Probably the best output, but will only work on a handful of editors and will require a lot of effort to implement.

I am leaning towards using showMessage and also logging the tree. Do others have opinions on other possibilities or pros/cons of the options i've listed above?

(sorry out today and haven't looked at code yet)

If it's a custom method, I think it should return the data as a json structure - the client already has to have custom support to invoke it, displaying the result isn't much extra work.

And I would really love to add a tree view to vscode, I think it wouldn't be hard (vs call hierarchy: no laziness and no direction-flipping) and could be reused for an AST viewer.

(sorry out today and haven't looked at code yet)

no worries it is a prototype, I wouldn't spend time looking at the implementation until we agree on the interaction :D
OTHO, checking out the lit test for output would probably be useful.

If it's a custom method, I think it should return the data as a json structure - the client already has to have custom support to invoke it, displaying the result isn't much extra work.

SGTM. WDYT about a json object in the form of:

interface MemoryTreeNode {
  name: string;
  totalSize: int;
  children?: Node[];
};

And I would really love to add a tree view to vscode, I think it wouldn't be hard (vs call hierarchy: no laziness and no direction-flipping) and could be reused for an AST viewer.

right, vscode already has APIs for it, https://code.visualstudio.com/api/extension-guides/tree-view.

kadircet updated this revision to Diff 298460.Oct 15 2020, 2:03 PM
  • As discussed offline moving with compact version of the output, while preserving names starting with an _ to be leaves.

This is also ready for an implementation-wise review now.

This looks really nice!

clang-tools-extra/clangd/ClangdLSPServer.cpp
615

can you add {"memoryTreeProvider", true} // clangd extension?

I'd like to add client support in VSCode and it's useful if the command is only visible with versions of clangd that support it.

1389

Putting the serialization code inline is a tempting option here because it's (mostly) just a map.
It's not something we do often though, so it does bury the protocol details in an unexpected place.

A couple of alternatives to consider:

  • map to a struct UsageTree { unsigned Self; unsigned Total; Map<string, UsageTree> Children; } defined in protocol.h, and define marshalling in the usual way
  • skip the struct and use MemoryTree as the type, providing a toJSON(const MemoryTree&) function in protocol.h

I lean towards the last alternative because it provides a bit more regularity without adding much more redundancy, but this is up to you.

1391

nit: Alloc -> DetailAlloc

1458

I think "dump" is a bit casual and also redundant - LSP methods to obtain information tend to have noun names. (executeCommand is the exception that proves the rule - it's used for side effects).

I went through the same question with the AST-dumping and landed on textDocument/ast as my preferred option, for what it's worth.

clang-tools-extra/clangd/ClangdLSPServer.h
145

(if you decide to move toJSON to protocol.h, then the second sentence onward probably belongs there)

kadircet updated this revision to Diff 298578.Oct 16 2020, 3:30 AM
kadircet marked 5 inline comments as done.
  • Add memoryTreeProvider to initialization params
  • Move serialization logic to Protocol.h and return MemoryTree directly.
  • Drop dump from method name.
sammccall accepted this revision.Oct 16 2020, 8:27 AM
sammccall added inline comments.
clang-tools-extra/clangd/ClangdLSPServer.cpp
623

one last naming nit: I think memoryUsage would put appropriately more weight on the semantics rather than the data structure

This revision is now accepted and ready to land.Oct 16 2020, 8:27 AM
kadircet updated this revision to Diff 298644.Oct 16 2020, 8:56 AM

s/memoryTree/memoryUsage/g

This revision was landed with ongoing or failed builds.Oct 19 2020, 3:31 AM
This revision was automatically updated to reflect the committed changes.