Page MenuHomePhabricator

[ASTMatchers] Don't garble the profiling output when multiple TU's are processed
AbandonedPublic

Authored by lebedev.ri on Apr 22 2018, 10:46 AM.

Details

Reviewers
sbenza
alexfh
Summary

D5911 added support for profiling clang-tidy checks.

It works nice, the tabulated report generated by clang-tidy -enable-check-profile is readable.
Unfortunately, it gets complicated with more than just one source file.

You could run clang-tidy on each file, then parse the profiles,
combine (sum) based on the Name column, and re-display.
Given that the profiles are display friendly, this is messy. (or is there a tool i've missed?)

Or, you could run clang-tidy only once, on all the sources at at once.
This obviously does not scale well. But one would think at least
it would sidestep the problem of combining the separate reports.

One would think. No, it does not. The profiling info is displayed,
only once at the end of clang-tidy run, but the info is garbage.
It only contains some portion of the data. I suspect it only contains the last TU.

This is because MatchASTVisitor is kinda smart, it contains it's own local
llvm::StringMap<llvm::TimeRecord> TimeByBucket;, which is used,
and at the end, MatchASTVisitor::~MatchASTVisitor() "propagates"
the collected data to the specified profiling info storage.
But it overrides it, not appends/combines...

Diff Detail

Repository
rC Clang

Event Timeline

lebedev.ri created this revision.Apr 22 2018, 10:46 AM

From a user's perspective I'd probably prefer a different behavior of checks profiling with multiple translation units: per-file table after each file and an aggregate table at the end. An independent improvement could be to support TSV/CSV output and/or dumping to a file to spare the user from parsing the tables out of the stdout/stderr.

Thank you for looking at this.

From a user's perspective I'd probably prefer a different behavior of checks profiling with multiple translation units: per-file table after each file and an aggregate table at the end.

Is this a review note, or a general observation?

I'm sure it could be done, i'm just not too sure how useful it would be, since it seems no one before now even noticed that timing with multiple TU's was 'broken'.

An independent improvement could be to support TSV/CSV output and/or dumping to a file to spare the user from parsing the tables out of the stdout/stderr.

Yes, and a script to merge those CSV's, would be nice.

Thank you for looking at this.

From a user's perspective I'd probably prefer a different behavior of checks profiling with multiple translation units: per-file table after each file and an aggregate table at the end.

Is this a review note, or a general observation?

I'm sure it could be done, i'm just not too sure how useful it would be, since it seems no one before now even noticed that timing with multiple TU's was 'broken'.

Hi @alexfh. Do i need to make those changes or not?
I'd really prefer to have such kind of high-level feedback the sooner the better, to avoid wasting everyone's time.

An independent improvement could be to support TSV/CSV output and/or dumping to a file to spare the user from parsing the tables out of the stdout/stderr.

Yes, and a script to merge those CSV's, would be nice.

Thank you for looking at this.

From a user's perspective I'd probably prefer a different behavior of checks profiling with multiple translation units: per-file table after each file and an aggregate table at the end.

Is this a review note, or a general observation?

Why not both? ;)

An independent improvement could be to support TSV/CSV output and/or dumping to a file to spare the user from parsing the tables out of the stdout/stderr.

Yes, and a script to merge those CSV's, would be nice.

I'd probably go with a set of features enough for various use cases:
0. don't add any profile merging logic to clang-tidy

  1. dump profile after each TU to the screen in the current tabulated format
  2. add a flag to specify a file name prefix to dump profile output for each file as CSV
  3. (optional) add a script to merge profiles from CSV files and dump as CSV or tabulated (without a script this could be done in a spreadsheet)

WDYT?

Thank you for looking at this.

From a user's perspective I'd probably prefer a different behavior of checks profiling with multiple translation units: per-file table after each file and an aggregate table at the end.

Is this a review note, or a general observation?

Why not both? ;)

Review latency, finite time amount, you name it :)

An independent improvement could be to support TSV/CSV output and/or dumping to a file to spare the user from parsing the tables out of the stdout/stderr.

Yes, and a script to merge those CSV's, would be nice.

I'd probably go with a set of features enough for various use cases:
0. don't add any profile merging logic to clang-tidy

  1. dump profile after each TU to the screen in the current tabulated format
  2. add a flag to specify a file name prefix to dump profile output for each file as CSV
  3. (optional) add a script to merge profiles from CSV files and dump as CSV or tabulated (without a script this could be done in a spreadsheet)

    WDYT?

Thank you for looking at this.

From a user's perspective I'd probably prefer a different behavior of checks profiling with multiple translation units: per-file table after each file and an aggregate table at the end.

Is this a review note, or a general observation?

Why not both? ;)

BTW, that did not answer the question:

Thank you for looking at this.

From a user's perspective I'd probably prefer a different behavior of checks profiling with multiple translation units: per-file table after each file and an aggregate table at the end.

Is this a review note, or a general observation?

I'm sure it could be done, i'm just not too sure how useful it would be, since it seems no one before now even noticed that timing with multiple TU's was 'broken'.

Hi @alexfh. Do i need to make those changes or not?
I'd really prefer to have such kind of high-level feedback the sooner the better, to avoid wasting everyone's time.

alexfh added a comment.May 4 2018, 5:31 AM

Thank you for looking at this.

From a user's perspective I'd probably prefer a different behavior of checks profiling with multiple translation units: per-file table after each file and an aggregate table at the end.

Is this a review note, or a general observation?

Why not both? ;)

BTW, that did not answer the question:

I'm sure it could be done, i'm just not too sure how useful it would be, since it seems no one before now even noticed that timing with multiple TU's was 'broken'.

I think, I noticed incorrect handling of profiling with multiple TUs at some point and switched to running clang-tidy multiple times. The solution I proposed (most importantly, point 1) would be the most convenient one for me at that time:

I'd probably go with a set of features enough for various use cases:

0. don't add any profile merging logic to clang-tidy

  1. dump profile after each TU to the screen in the current tabulated format
  2. add a flag to specify a file name prefix to dump profile output for each file as CSV
  3. (optional) add a script to merge profiles from CSV files and dump as CSV or tabulated (without a script this could be done in a spreadsheet)

Are any other questions still open?

alexfh requested changes to this revision.May 4 2018, 10:52 AM
This revision now requires changes to proceed.May 4 2018, 10:52 AM
lebedev.ri abandoned this revision.Fri, Jun 21, 8:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jun 21, 8:49 AM