This is an archive of the discontinued LLVM Phabricator instance.

Refactor Matcher<T> and DynTypedMatcher to reduce overhead of casts.
ClosedPublic

Authored by sbenza on Sep 24 2014, 1:53 PM.

Details

Summary

This change introduces DynMatcherInterface and changes the internal
representation of DynTypedMatcher and Matcher<T> to use a generic
interface instead.
It removes unnecessary indirections and virtual function calls when
converting matchers by implicit and dynamic casts.
DynTypedMatcher now remembers the stricter type in the chain of casts
and checks it before calling into DynMatcherInterface.
This change improves our clang-tidy related benchmark by ~14%.
Also, it opens the door for more optimizations of this kind that are
coming in future changes.

As a side effect of removing these template instantiations, it also
speeds up compilation of Dynamic/Registry.cpp by ~17% and reduces the number of
symbols generated by ~30%.

Diff Detail

Event Timeline

sbenza updated this revision to Diff 14051.Sep 24 2014, 1:53 PM
sbenza retitled this revision from to Refactor Matcher<T> and DynTypedMatcher to reduce overhead of casts..
sbenza updated this object.
sbenza edited the test plan for this revision. (Show Details)
sbenza added a reviewer: klimek.
sbenza added a subscriber: Unknown Object (MLST).

fyi, this change moves the DynTypedMatcher class from below Matcher<> to above it, so the diff is a little confusing.

klimek accepted this revision.Sep 25 2014, 1:10 AM
klimek edited edge metadata.

Wizardry! Nice benchmark numbers :)

lib/ASTMatchers/ASTMatchersInternal.cpp
54–60

Can we somehow put this implementation and the IdMatcher implementation closer together? I'm concerned if somebody changes one and the other breaks.

95–96

"mismatches" or "a mismatch"?

This revision is now accepted and ready to land.Sep 25 2014, 1:10 AM

fyi, I think this breaks memoization. The ID is now not enough to uniquely identify a matcher. The RestrictKind should also be part of it.
I will investigate before submitting this change.

lib/ASTMatchers/ASTMatchersInternal.cpp
54–60

My plan is to refector all template matcher that do not need the type into DynMatcherInterface. That includes IdMatcher, TrueMatcher, VariadicOperator, etc.
I did not do it on this change to limit the scope. Added some FIXME for them, though.

klimek added inline comments.Sep 25 2014, 6:46 AM
lib/ASTMatchers/ASTMatchersInternal.cpp
54–60

sg

sbenza updated this revision to Diff 14075.Sep 25 2014, 9:17 AM
sbenza edited edge metadata.

Added the RestrictKind to the matcher ID.
Also added a regression test to make sure memoization works with the new changes.
Verified that the test failed before the fix and passed afterwards.

sbenza updated this revision to Diff 14076.Sep 25 2014, 9:20 AM

Minor fixes

lib/ASTMatchers/ASTMatchersInternal.cpp
95–96

Fixed.

klimek added inline comments.Sep 25 2014, 11:50 AM
include/clang/ASTMatchers/ASTMatchersInternal.h
288

Can you expand the comment on why we need to return a pair, so we remember 2 weeks from now :)

sbenza updated this revision to Diff 14175.Sep 29 2014, 10:07 AM

Added comment

include/clang/ASTMatchers/ASTMatchersInternal.h
288

Done.

klimek added inline comments.Sep 29 2014, 10:38 AM
include/clang/ASTMatchers/ASTMatchersInternal.h
290–292

Can you give an example in the comment? I find it not trivial to reason about the actual cases this affects.

sbenza updated this revision to Diff 14182.Sep 29 2014, 10:53 AM

Comment

include/clang/ASTMatchers/ASTMatchersInternal.h
290–292

How about now?

Perfect. Thanks. LG

sbenza closed this revision.Sep 29 2014, 11:53 AM
sbenza updated this revision to Diff 14183.

Closed by commit rL218616 (authored by @sbenza).

Excuse me, I reverted it in r218648, for now.

  • MSC17 doesn't compile it.

    clang/include/clang/ASTMatchers/ASTMatchersInternal.h(387) : error C4519: default template arguments are only allowed on a class template