[PATCH] D99110: [Coverage] Load records immediately

[Choongwoo Han via Phabricator via llvm-commits]

tunz added a comment.

In D99110#2642603 <https://reviews.llvm.org/D99110#2642603>, @vsk wrote:

> Have you determined why opening a readonly binary costs memory?
>
> I think I see the problem, have you tried disabling the RequiresNullTerminator bit?
>
>   diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
>   index cdbcde50d33a..f34e29397b3a 100644
>   --- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
>   +++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
>   @@ -318,11 +318,12 @@ CoverageMapping::load(ArrayRef<StringRef> ObjectFilenames,
>      auto ProfileReader = std::move(ProfileReaderOrErr.get());
>   
>      SmallVector<std::unique_ptr<CoverageMappingReader>, 4> Readers;
>      SmallVector<std::unique_ptr<MemoryBuffer>, 4> Buffers;
>      for (const auto &File : llvm::enumerate(ObjectFilenames)) {
>   -    auto CovMappingBufOrErr = MemoryBuffer::getFileOrSTDIN(File.value());
>   +    auto CovMappingBufOrErr = MemoryBuffer::getFileOrSTDIN(
>   +        File.value(), /*FileSize=*/-1, /*RequiresNullTerminator=*/false);
>        if (std::error_code EC = CovMappingBufOrErr.getError())
>          return errorCodeToError(EC);
>        StringRef Arch = Arches.empty() ? StringRef() : Arches[File.index()];
>        MemoryBufferRef CovMappingBufRef =
>            CovMappingBufOrErr.get()->getMemBufferRef();
>
> This should prevent the MemoryBuffer API from allocating a buffer for the object file, just to add a '\0' at the end. If the OS can just mmap() the buffer readonly, that should be essentially free.

Yes, I have tried disabling the RequiresNullTerminator bit, but it didn't help. Actually, the readonly buffer itself was fine. There are some places reading and storing data from them.
For example, https://github.com/llvm/llvm-project/blob/95f7f7c21b47f1739e4a901d428af970b7d7c0b9/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp#L980

  std::string FuncRecords;
  ...
  for (SectionRef Section : *CoverageRecordsSections) {
    auto CoverageRecordsOrErr = Section.getContents();
    if (!CoverageRecordsOrErr)
      return CoverageRecordsOrErr.takeError();
    FuncRecords += CoverageRecordsOrErr.get();
    while (FuncRecords.size() % 8 != 0)
      FuncRecords += '\0';
  }

FuncRecords is stored as a new string. When I removed this part, I could save about 10-20% of memory. And, there are some other similar things such as uncompressed data. So, I just came back to this approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99110