This is an archive of the discontinued LLVM Phabricator instance.

Add support for profiling the matchers used.
ClosedPublic

Authored by sbenza on Oct 22 2014, 9:18 AM.

Details

Summary

Add support for profiling the matchers used.
This will be connected with clang-tidy to generate a report to determine
and debug slow checks.

Diff Detail

Event Timeline

sbenza updated this revision to Diff 15247.Oct 22 2014, 9:18 AM
sbenza retitled this revision from to Add support for profiling the matchers used..
sbenza updated this object.
sbenza edited the test plan for this revision. (Show Details)
sbenza added a reviewer: alexfh.
sbenza added a subscriber: Unknown Object (MLST).
alexfh accepted this revision.Oct 22 2014, 10:21 AM
alexfh edited edge metadata.

Awesome! A couple of comments inline.

include/clang/ASTMatchers/ASTMatchFinder.h
125

I'd prefer a more structured interface, e.g. a map from string to some struct containing collected timings + a method to convert this map to string or print it to a stream. Not sure if this fits well with LLVM timing facilities you use, though.

lib/ASTMatchers/ASTMatchFinder.cpp
312

You can check EnableCheckProfiling inside getTimerForBucket to reduce code repetition.

This revision is now accepted and ready to land.Oct 22 2014, 10:21 AM
sbenza added inline comments.Oct 22 2014, 10:35 AM
include/clang/ASTMatchers/ASTMatchFinder.h
125

The current TimerGroup interface is a little awkward.
TimerGroup will print to llvm::errs() when the last timer is destroyed, unless you print() yourself to your own ostream.
I could skip the TimerGroup class and export the TimeRecords directly.
You could not use the TimerGroup printing logic, though. Most of it comes from TimeRecord anyway, so it wouldn't be that bad.

So, would you prefer if this was:

struct ProfileChecks {

llvm::StringMap<llvm::TimeRecord> *Records;

};

?

lib/ASTMatchers/ASTMatchFinder.cpp
312

I put it outside to avoid calling the function when disabled.

sbenza updated this revision to Diff 15264.Oct 22 2014, 11:34 AM
sbenza edited edge metadata.

Export the raw TimeRecord elements.

This is even better. One nit below.

include/clang/ASTMatchers/ASTMatchFinder.h
134

Maybe just "CheckProfiling"? "Enable" hints that this is a bool, but it's not (though is used as a bool in some places).

sbenza updated this revision to Diff 15267.Oct 22 2014, 11:59 AM

Fix some names.

include/clang/ASTMatchers/ASTMatchFinder.h
134

It used to be a bool on the first iterations of the code.
Then it evolved into having the ostream.
And now has the record map =)

Looks good!

sbenza closed this revision.Oct 22 2014, 1:41 PM
sbenza updated this revision to Diff 15273.

Closed by commit rL220418 (authored by @sbenza).