Page MenuHomePhabricator

[clangd] Support configuration of inlay hints.
ClosedPublic

Authored by sammccall on Jan 5 2022, 5:01 PM.

Details

Summary

The idea is that the feature will always be advertised at the LSP level, but
depending on config we'll return partial or no responses.

We try to avoid doing the analysis for hints we're not going to return.

Examples of syntax:

# No hints
InlayHints:
  Enabled: No
---
# Turn off a default category
InlayHints:
  ParameterNames: No
---
# Turn on some categories
InlayHints:
  ParameterNames: Yes
  DeducedTypes: Yes

Diff Detail

Event Timeline

sammccall created this revision.Jan 5 2022, 5:01 PM
sammccall requested review of this revision.Jan 5 2022, 5:01 PM
hokein added inline comments.Jan 6 2022, 11:53 PM
clang-tools-extra/clangd/InlayHints.cpp
54

this should be !Cfg.InlayHints.ParameterNames.

What do you think the idea of moving guards deeper (processCall and addTypeHint)? The code seems clearer and we don't have to add them in all Visit* implementation, this means that we pay cost of running some necessary code, but I think it is trivial and probably worthy.

69

!Cfg.InlayHints.ParameterNames

sammccall updated this revision to Diff 398228.Jan 7 2022, 1:34 PM
sammccall marked 2 inline comments as done.

Simplify & fix category checks

sammccall added inline comments.Jan 7 2022, 3:04 PM
clang-tools-extra/clangd/InlayHints.cpp
54

I agree where to put the checks is an annoying question (and worth minimizing, since I've managed to get two wrong already).
I do think there needs to be a pattern so we don't accidentally skip checks.

  • Easest is to postfilter (check in addInlayHint). That *does* hurt performance. Debug build on my laptop is ~170ms for all hints, ~160ms for postfilter with all categories disabled, ~5ms for current impl (prefilter) with all categories disabled. (On SemaCodeComplete.cpp, just eyeballing traces)
  • Checking in VisitXXX methods (as done here) is a very regular pattern. Downside is you need many checks, and you can forget/break one
  • Checks in helpers so that one is always hit (as you propose) touches fewer places but it's ad-hoc. I'm afraid of getting it wrong as the impl is modified (missing checks, doing too much work first, etc).
  • Splitting RAV per category is an interesting option. Checking is very elegant, nicer code structure, can trace per-category latency, disabled categories can't crash... The downside is extra overhead, but this must be <5ms in the performance above. We can still choose to bundle simple categories together...

I think I'll do as you propose for now, improve the tests, and refactor to try to make it feel less ad-hoc later.
Also I should work out what we're spending 170ms on... EDIT: it turns out it's just assembling JSON objects :-\

hokein accepted this revision.Jan 10 2022, 12:57 AM
hokein added inline comments.
clang-tools-extra/clangd/InlayHints.cpp
54

Thanks for digging into this.

~5ms for current impl (prefilter) with all categories disabled. (On SemaCodeComplete.cpp, just eyeballing traces)

Yeah, we should have this prefilter implementation when all categories are disabled.

Debug build on my laptop is ~170ms for all hints, ~160ms for postfilter with all categories disabled,

The data doesn't seem reasonable, I think the total cost comes from two main sources (AST traversal cost + assembling JSON object cost),
I would expect the allhints and postfilter have the similar AST traversal cost, but postfilter should have a near zero cost of assembling JSON objectcs (as it returns an empty vector), so the AST traversal cost is ~160ms.

Also I should work out what we're spending 170ms on... EDIT: it turns out it's just assembling JSON objects :-\

And based on this, it turns out the cost of all-hints approach should be larger than 170ms, which should be ~300ms (160ms for AST traversal + 170ms for assembling JSON objects).

Anyway, the current implementation looks good.

This revision is now accepted and ready to land.Jan 10 2022, 12:57 AM
This revision was automatically updated to reflect the committed changes.

Assuming origin/maain is a branch typo and is no longer being tracked, would you please delete the upstream one in https://github.com/llvm/llvm-project/branches?