[PATCH] D122121: [clang][dataflow] Add action caching support to MatchSwitch

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D122121#3396688 <https://reviews.llvm.org/D122121#3396688>, @xazax.hun wrote:

> Thanks! Did you have a chance whether this makes a difference in real world scenarios? I'm mostly curious because I do not have a good mental model of how the matchers are implemented, specifically what optimizations are in place, so I don't really know how much of an impact can caching make :)

In general, for clang-tidy style use, we've found that building the AST dwarfs the cost of the matchers, so there's limited room for performance improvements from the matchers themselves.  That said, the `hasName` matcher is optimized in the case of qualified names, and I expect that was motivated by noticable performance costs from the easy version of just printing the fully qualified name of the type and comparing the strings.

The complication from caching is that it can only be done when we're sure that the name is unique. So, either its a fully qualified name, or the user (of the matcher) tells us explicitly that it can be cached. That might make the `hasName` interface a little clunkier, but maybe its worth it.

> Once the rest of the comments are resolved this looks good to me.
>
> In D122121#3396149 <https://reviews.llvm.org/D122121#3396149>, @ymandel wrote:
>
>> In D122121#3396096 <https://reviews.llvm.org/D122121#3396096>, @sgatev wrote:
>>
>>> I was thinking that maybe we can split the problem in two:
>>>
>>> 1. duplicated work across `matchDynamic` calls, and
>>> 2. duplicated work within a single `matchDynamic` call.
>>>
>>> This patch addresses 1. as I see it as responsibility of `MatchSwitch`. Your comment seems to be about 2., if I'm reading it correctly.
>>
>> Right. This division makes sense, particularly the point about responsibility. Point 2 is very much independent of `MatchSwitch`. But, I think matching the `optional` type is about both really in that every time we call the matcher we'll be checking the optional type, even if we only check it once *within* a single match. Still, fixing it doen't belong in `MatchSwitch`.
>
> +1, I like how the problem was split. I also agree that `MatchSwitch` might not be the best place to fix #2 as a more broad fix would potentially benefit other parts of Clang, like Tidy.  But speaking of other parts of Clang, I wonder whether `MatchSwitch` is something that could be part of the ASTMatcher library. Some people might find it useful for implementing tools other than flow sensitive checks. Although, I do not have a strong preference, I'm completely fine leaving it where it currently is.

I'll check w/ Aaron Ballman, one of the main contributors/reviewers for matchers these days.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122121/new/

https://reviews.llvm.org/D122121