This is an archive of the discontinued LLVM Phabricator instance.

Speeding up llvm-cov export with multithreaded renderFiles implementation.
ClosedPublic

Authored by sajjadm on Mar 12 2019, 4:05 PM.

Details

Summary

CoverageExporterJson::renderFiles accounts for most of the execution time given a large profdata file with multiple binaries.

Proposed solution is to generate JSON for each file in parallel and sort at the end to preserve deterministic output. Also added flags to skip generating parts of the output to trim the output size.

Patch by Sajjad Mirza (@sajjadm).

Event Timeline

sajjadm created this revision.Mar 12 2019, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2019, 4:05 PM
sajjadm edited the summary of this revision. (Show Details)Mar 12 2019, 4:07 PM
Dor1s requested changes to this revision.Mar 13 2019, 9:53 AM

Thanks for the patch, @sajjadm ! I left some comments. You would also need to update the documentation page and add a test for the new behavior you're introducing.

llvm/tools/llvm-cov/CoverageExporterJson.cpp
137

Maybe we should pass the whole ViewOpts object here now, rather than individual flags? Same for renderFiles

202

It's unclear what exactly this function returns. From the declaration it feels like it will return true for equal Json objects and false otherwise. However, it doesn't seem to be the case. Could you please rename it to be more explicit?

205–206

update the comment to something like "Skip functions-level information if necessary".

211

Let's pass the whole Options object and create the Pool inside renderFiles.

This revision now requires changes to proceed.Mar 13 2019, 9:53 AM

Have we written tests for the new behaviors?

vsk added inline comments.Mar 13 2019, 10:37 AM
llvm/tools/llvm-cov/CoverageExporterJson.cpp
213

As this is the only use of the comparator, consider passing in a lambda which consumes const json::Object & parameters.

Have we written tests for the new behaviors?

Not sure who this question was addressed to, but yeah we've been always adding/modifying tests when changing something, e.g. adding a new flag to llvm-cov export (https://reviews.llvm.org/D41085) or your fix for symlink issue (https://reviews.llvm.org/D44960).

Have we written tests for the new behaviors?

Not sure who this question was addressed to, but yeah we've been always adding/modifying tests when changing something, e.g. adding a new flag to llvm-cov export (https://reviews.llvm.org/D41085) or your fix for symlink issue (https://reviews.llvm.org/D44960).

Sorry for the confusion, it was to sajjadm@ for this change.

sajjadm updated this revision to Diff 190531.Mar 13 2019, 4:12 PM
  • Added tests for the -skip-* flags.
  • Minor changes in response to code review.
sajjadm marked 5 inline comments as done.Mar 13 2019, 4:18 PM

Thanks for all the feedback, Max and Vedant!

@Dor1s , do you still find the comparison function confusing now that it's a lambda? It's meant to return a boolean in the manner expected by std::sort. True if the left argument is "less" than the right argument, and false otherwise.

vsk added a comment.Mar 13 2019, 4:40 PM

Thanks for working on this!

llvm/tools/llvm-cov/CoverageExporterJson.cpp
158

Can the parallelism be controlled by -num-threads/-j, similar to: https://llvm.org/docs/CommandGuide/llvm-cov.html#cmdoption-llvm-cov-show-num-threads

This will help write a test that the output from e.g. -j 4 matches the output from -j 1.

sajjadm added inline comments.Mar 13 2019, 5:05 PM
llvm/tools/llvm-cov/CoverageExporterJson.cpp
158

Yes, this code respects -num-threads. There's actually a test that already does the check you're describing: multithreaded-report.test.

sajjadm updated this revision to Diff 190546.Mar 13 2019, 5:06 PM
  • Added docs for new llvm-cov flags.
sajjadm updated this revision to Diff 190547.Mar 13 2019, 5:13 PM
sajjadm marked 2 inline comments as done.
  • Changed how -num-threads is documented.

I was debating whether I should pull -num-threads out of the docs for show and move it to the top-level options section, or make a copy of it in the export options section. I went with putting a copy of it in export because only show and export use it and the behavior is subtly different (show only uses it if -output-dir is set, export uses it unconditionally).

Dor1s accepted this revision.Mar 14 2019, 7:24 AM

Thanks for all the feedback, Max and Vedant!

@Dor1s , do you still find the comparison function confusing now that it's a lambda? It's meant to return a boolean in the manner expected by std::sort. True if the left argument is "less" than the right argument, and false otherwise.

As lambda comparator it totally makes sense.

The change looks good to me! Let's wait for a final approval by @vsk and then I can commit it for you.

This revision is now accepted and ready to land.Mar 14 2019, 7:24 AM
vsk accepted this revision.Mar 14 2019, 9:11 AM

Thanks, lgtm.

Dor1s updated this revision to Diff 190667.Mar 14 2019, 10:46 AM

Verified that the tests are passing, getting ready to commit.

Dor1s edited the summary of this revision. (Show Details)Mar 14 2019, 10:46 AM
Dor1s edited the summary of this revision. (Show Details)Mar 14 2019, 10:47 AM
Dor1s retitled this revision from Speeding up llvm-cov with multithreaded renderFiles. to Speeding up llvm-cov export with multithreaded renderFiles implementation..
This revision was automatically updated to reflect the committed changes.