[PATCH] D99110: [Coverage] Load records immediately

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

Thanks for bearing with me :). The memory profile is really helpful.

Based on the data, I think you should go ahead and fold in the /*RequiresNullTerminator=*/false change (looks like another 1gb savings).

Replacing `std::string FuncRecords` sounds more involved, that can happen in a follow up patch if necessary.



================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:284
 
-Expected<std::unique_ptr<CoverageMapping>> CoverageMapping::load(
+Error CoverageMapping::loadFromReaders(
     ArrayRef<std::unique_ptr<CoverageMappingReader>> CoverageReaders,
----------------
Please add a comment here explaining this this memory optimization works by shortening the lifetimes of CoverageMappingReader instances.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:286
     ArrayRef<std::unique_ptr<CoverageMappingReader>> CoverageReaders,
-    IndexedInstrProfReader &ProfileReader) {
-  auto Coverage = std::unique_ptr<CoverageMapping>(new CoverageMapping());
-
+    IndexedInstrProfReader &ProfileReader, CoverageMapping *Coverage) {
   for (const auto &CoverageReader : CoverageReaders) {
----------------
This should accept a `CoverageMapping &`, that clearly conveys that Coverage cannot be null.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:290
       if (Error E = RecordOrErr.takeError())
         return std::move(E);
       const auto &Record = *RecordOrErr;
----------------
Since this returns Error, the std::move(E) now triggers -Wpessimizing-move, so you can just drop it. I guess it was needed earlier so the implicit Expected(Error &&) constructor could be invoked.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:293
       if (Error E = Coverage->loadFunctionRecord(Record, ProfileReader))
         return std::move(E);
     }
----------------
Ditto.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:350
+    DataFound |= !Readers.empty();
+    if (Error E = loadFromReaders(Readers, *ProfileReader, Coverage.get()))
+      return std::move(E);
----------------
Simply passing `*Coverage` works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99110