hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:51
bool VisitCXXConstructExpr(CXXConstructExpr *E) {
+ if (!Cfg.InlayHints.DeducedTypes)
+ return true;
----------------
sammccall wrote:
> hokein wrote:
> > 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.
> 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 :-\
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116713/new/
https://reviews.llvm.org/D116713
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
