[PATCH] D59277: Speeding up llvm-cov with multithreaded renderFiles.

[Max Moroz via Phabricator via llvm-commits]

Dor1s requested changes to this revision.
Dor1s added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: llvm/tools/llvm-cov/CoverageExporterJson.cpp:137
                         const FileCoverageSummary &FileReport,
-                        bool ExportSummaryOnly) {
+                        bool ExportSummaryOnly, bool SkipExpansions) {
   json::Object File({{"filename", Filename}});
----------------
Maybe we should pass the whole `ViewOpts` object here now, rather than individual flags? Same for `renderFiles`


================
Comment at: llvm/tools/llvm-cov/CoverageExporterJson.cpp:197
 
+static bool compareFileJson(const json::Value &A, const json::Value &B) {
+  const json::Object *ObjA = A.getAsObject();
----------------
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?


================
Comment at: llvm/tools/llvm-cov/CoverageExporterJson.cpp:218
+  ThreadPool Pool(NumThreads);
+  auto Files = renderFiles(Coverage, SourceFiles, FileReports, &Pool,
+                           Options.ExportSummaryOnly, Options.SkipExpansions);
----------------
Let's pass the whole `Options` object and create the `Pool` inside `renderFiles`.


================
Comment at: llvm/tools/llvm-cov/CoverageExporterJson.cpp:223
+      {{"files", std::move(Files)}, {"totals", renderSummary(Totals)}});
   // Skip functions-level information for summary-only export mode.
+  if (!Options.ExportSummaryOnly && !Options.SkipFunctions)
----------------
update the comment to something like "Skip functions-level information if necessary".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59277/new/

https://reviews.llvm.org/D59277