Page MenuHomePhabricator

[llvm-cov] Multi-threaded implementation of prepareFileReports method.
ClosedPublic

Authored by Dor1s on Dec 13 2017, 4:10 PM.

Details

Summary

Local testing has demonstrated a great speed improvement, compare the following:

  1. Existing version:
$ time llvm-cov show -format=html -output-dir=report -instr-profile=... ...
The tool has been launched:                            00:00:00
Loading coverage data:                                 00:00:00
Get unique source files:                               00:00:33
Creating an index out of the source files:             00:00:34
Going into prepareFileReports:                         00:00:34
Going to emit summary information for each file:       00:28:55 <-- 28:21 min!
Going to emit links to files with no function:         00:28:55
Launching 32 threads for generating HTML files:        00:28:55

real  37m43.651s
user  112m5.540s
sys   7m39.872s
  1. Multi-threaded version with 32 CPUs:
$ time llvm-cov show -format=html -output-dir=report -instr-profile=... ...
The tool has been launched:                            00:00:00
Loading coverage data:                                 00:00:00
Get unique source files:                               00:00:38
Creating an index out of the source files:             00:00:40
Going into prepareFileReports:                         00:00:40
Preparing file reports using 32 threads:               00:00:40
# Creating thread tasks for the following number of files: 16422
Going to emit summary information for each file:       00:01:57 <-- 1:17 min!
Going to emit links to files with no function:         00:01:58
Launching 32 threads for generating HTML files:        00:01:58

real  11m2.044s
user  134m48.124s
sys   7m53.388s

Event Timeline

Dor1s created this revision.Dec 13 2017, 4:10 PM
Dor1s updated this revision to Diff 127608.Dec 19 2017, 3:13 PM

Another local version, that one seems to work...

Dor1s updated this revision to Diff 127614.Dec 19 2017, 3:39 PM

Some initial clean up, remove unnecessary code.

Dor1s updated this revision to Diff 127645.Dec 19 2017, 6:19 PM

More clean up.

Dor1s updated this revision to Diff 127647.Dec 19 2017, 6:24 PM

Removing more unnecessary code and rename an argument

Dor1s updated this revision to Diff 127737.Dec 20 2017, 8:39 AM

Remove another racy component

Dor1s retitled this revision from WIP: hacky local changes for speeding up llvm-cov HTML report generation. to [llvm-cov] Multi-threaded implementation of prepareFileReports method..Dec 20 2017, 8:42 AM
Dor1s edited the summary of this revision. (Show Details)
Dor1s added reviewers: vsk, morehouse.
Dor1s added a subscriber: kcc.
vsk added a comment.Dec 20 2017, 11:32 AM

Nice!

For a test, I suggest creating a report for >1 files with/without threading, and asserting that the reports are identical. It would be good to exercise the new summary aggregation logic by including different instantiations of the same template function in >1 files.

tools/llvm-cov/CoverageReport.cpp
365

IIUC, PartialTotals should contain exactly the same results as the FileReports vector after Pool.wait(). Why not get rid of it?

376

clang-format?

tools/llvm-cov/CoverageReport.h
48

Since StringRef is immutable and cheap to copy, could you pass it by value?

tools/llvm-cov/CoverageSummaryInfo.h
181

I don't think we should make it possible to create file summaries with empty names. Was this introduced so the std::vector could default-initialize FileCoverageSummary? If so, would make_unique<FileCoverageSummary[]>(N) suffice as an alternative?

186

If we stipulate that file summaries have valid names, we could avoid a bit of this complexity.

Dor1s marked 2 inline comments as done.Dec 21 2017, 2:38 PM

Thanks for the comments, Vedant! I have some questions now :)

tools/llvm-cov/CoverageReport.cpp
365

You're certainly right, thanks for catching!

tools/llvm-cov/CoverageSummaryInfo.h
181

Yes :( I'm not sure how make_unique may help, e.g. if I do:

auto FileReports = std::make_unique<FileCoverageSummary[]>(Files.size());

It still needs a default constructor. Did you mean llvm::make_unique by any chance? It still looks for an empty constructor though

186

Sure! But we still need to verify that RHS.Name == LHS.Name, right?

Dor1s marked an inline comment as done.Dec 21 2017, 2:44 PM
Dor1s added inline comments.
tools/llvm-cov/CoverageSummaryInfo.h
181

Either way, I found another solution. Let me upload it real quick so you can take a look :)

Dor1s updated this revision to Diff 127952.Dec 21 2017, 2:47 PM
Dor1s edited the summary of this revision. (Show Details)

Address review comments

Dor1s added a subscriber: Dor1s.Dec 21 2017, 2:50 PM
vsk added inline comments.Dec 21 2017, 2:50 PM
tools/llvm-cov/CoverageSummaryInfo.h
181

Looks good.

186

Right, that can't hurt.

Dor1s marked 2 inline comments as done.Dec 21 2017, 3:08 PM
Dor1s added inline comments.
tools/llvm-cov/CoverageSummaryInfo.h
186

Wait, I'm stupid... Removing this assert right now.

Dor1s updated this revision to Diff 127958.Dec 21 2017, 3:12 PM

Clean up, will add some tricky test later today or tomorrow

Dor1s updated this revision to Diff 128047.Dec 22 2017, 11:37 AM

Added the test, but other tests seem to be failing on my machine, updating my checkout now

Interesting. Actually, the new test seems to work fine, but bunch of others are failing with my change:

Failing Tests (15):
    LLVM :: tools/llvm-cov/binary-formats.c
    LLVM :: tools/llvm-cov/dir-with-filtering.test
    LLVM :: tools/llvm-cov/load-multiple-objects.test
    LLVM :: tools/llvm-cov/multiple-files.test
    LLVM :: tools/llvm-cov/multiple-objects.test
    LLVM :: tools/llvm-cov/prevent_false_instantiations.h
    LLVM :: tools/llvm-cov/report.cpp
    LLVM :: tools/llvm-cov/showExpansions.cpp
    LLVM :: tools/llvm-cov/showHighlightedRanges.cpp
    LLVM :: tools/llvm-cov/showLineExecutionCounts.cpp
    LLVM :: tools/llvm-cov/showRegionMarkers.cpp
    LLVM :: tools/llvm-cov/showTemplateInstantiations.cpp
    LLVM :: tools/llvm-cov/sources-specified.test
    LLVM :: tools/llvm-cov/universal-binary.c
    LLVM :: tools/llvm-cov/zeroFunctionFile.c

  Expected Passes    : 21
  Unsupported Tests  : 1
  Unexpected Failures: 15

with the following crash:

terminate called after throwing an instance of 'std::system_error'
  what():  Resource temporarily unavailable
/usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov[0x47e81a]
/usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov[0x47cb1e]
/usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov[0x47cc5a]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x10330)[0x7ff4e2fb5330]
/lib/x86_64-linux-gnu/libc.so.6(gsignal+0x37)[0x7ff4e1fb0c37]
/lib/x86_64-linux-gnu/libc.so.6(abort+0x148)[0x7ff4e1fb4028]
/usr/lib/x86_64-linux-gnu/libstdc++.so.6(_ZN9__gnu_cxx27__verbose_terminate_handlerEv+0x155)[0x7ff4e28bf535]
/usr/lib/x86_64-linux-gnu/libstdc++.so.6(+0x5e6d6)[0x7ff4e28bd6d6]
/usr/lib/x86_64-linux-gnu/libstdc++.so.6(+0x5e703)[0x7ff4e28bd703]
/usr/lib/x86_64-linux-gnu/libstdc++.so.6(+0x5e922)[0x7ff4e28bd922]
/usr/lib/x86_64-linux-gnu/libstdc++.so.6(_ZSt20__throw_system_errori+0x80)[0x7ff4e290f800]
/usr/lib/x86_64-linux-gnu/libstdc++.so.6(_ZNSt6thread15_M_start_threadESt10shared_ptrINS_10_Impl_baseEE+0x248)[0x7ff4e2910d68]
/usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov[0x461b55]
/usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov[0x434f16]
/usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov[0x435b37]
/usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov[0x4368d8]
/usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov[0x436aff]
/usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov[0x424c2b]
/usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov[0x42555b]
/usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov[0x40dd57]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5)[0x7ff4e1f9bf45]
/usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov[0x412567]
Stack dump:
0.	Program arguments: /usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov /usr/local/google/home/mmoroz/Projects/llvm/build/bin/llvm-cov report /usr/local/google/home/mmoroz/Projects/llvm/llvm/test/tools/llvm-cov/Inputs/report.covmapping -instr-profile /usr/local/google/home/mmoroz/Projects/llvm/llvm/test/tools/llvm-cov/Inputs/report.profdata -path-equivalence=/tmp,/usr/local/google/home/mmoroz/Projects/llvm/llvm/test/tools/llvm-cov -show-region-summary -show-instantiation-summary 
Aborted (core dumped)

I've certainly broken something, will debug.

Dor1s updated this revision to Diff 128056.Dec 22 2017, 2:36 PM

Now only one test is failing, continue debugging.

Dor1s marked 8 inline comments as done.Dec 22 2017, 2:50 PM
Dor1s updated this revision to Diff 128058.Dec 22 2017, 3:05 PM

Woohoo, everything works!

Dor1s updated this revision to Diff 128059.Dec 22 2017, 3:13 PM

Minor changes

Dor1s added a comment.Dec 22 2017, 3:19 PM

All comments have been addressed + a test have been added.

tools/llvm-cov/CoverageReport.h
53

I don't like having raw pointers here, but I can't get it to work otherwise. The most recent issue I had: Filters were not applied, when I've been using const reference. Once I changed that to a pointer, it started to work. Is there any better pattern of using ThreadPool?

Dor1s added a comment.Jan 3 2018, 12:36 PM

Happy New Year and friendly ping, if you guys got back to work already :)

LGTM, but let Vedant comment since he's more familiar with this code.

tools/llvm-cov/CoverageReport.h
53

This is probably caused by pass-by-reference issues with the std::bind that occurs in the multi-parameter ThreadPool::async. You could try using a lambda that captures the arguments to prepareSingleFileReport and then calls it with those arguments. That should let you use the single-parameter ThreadPool::async and avoid the bind.

Otherwise, pointers work too.

vsk accepted this revision.Jan 3 2018, 4:08 PM

Thanks, LGTM.

tools/llvm-cov/CoverageReport.h
53

Right, ThreadPool will try to make a copy of data pointed-to by a captured reference, which won't compile for the move-only filters. Using pointers here sounds fine.

This revision is now accepted and ready to land.Jan 3 2018, 4:08 PM
Dor1s updated this revision to Diff 128751.Jan 5 2018, 8:15 AM

Re-build test files

Dor1s closed this revision.Jan 5 2018, 8:16 AM