[PATCH] D62623: Reduce memory consumption of coverage dumps

[serge via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4cd07dbeec98: Reduce memory consumption of coverage dumps 
(authored by serge-sans-paille).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62623

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1388,10 +1388,19 @@
   std::string FilenamesAndCoverageMappings;
   llvm::raw_string_ostream OS(FilenamesAndCoverageMappings);
   CoverageFilenamesSectionWriter(FilenameRefs).write(OS);
-  std::string RawCoverageMappings =
-  llvm::join(CoverageMappings.begin(), CoverageMappings.end(), "");
-  OS << RawCoverageMappings;
-  size_t CoverageMappingSize = RawCoverageMappings.size();
+
+  // Stream the content of CoverageMappings to OS while keeping
+  // memory consumption under control.
+  size_t CoverageMappingSize = 0;
+  for (auto  : CoverageMappings) {
+CoverageMappingSize += S.size();
+OS << S;
+S.clear();
+S.shrink_to_fit();
+  }
+  CoverageMappings.clear();
+  CoverageMappings.shrink_to_fit();
+
   size_t FilenamesSize = OS.str().size() - CoverageMappingSize;
   // Append extra zeroes if necessary to ensure that the size of the filenames
   // and coverage mappings is a multiple of 8.


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1388,10 +1388,19 @@
   std::string FilenamesAndCoverageMappings;
   llvm::raw_string_ostream OS(FilenamesAndCoverageMappings);
   CoverageFilenamesSectionWriter(FilenameRefs).write(OS);
-  std::string RawCoverageMappings =
-  llvm::join(CoverageMappings.begin(), CoverageMappings.end(), "");
-  OS << RawCoverageMappings;
-  size_t CoverageMappingSize = RawCoverageMappings.size();
+
+  // Stream the content of CoverageMappings to OS while keeping
+  // memory consumption under control.
+  size_t CoverageMappingSize = 0;
+  for (auto  : CoverageMappings) {
+CoverageMappingSize += S.size();
+OS << S;
+S.clear();
+S.shrink_to_fit();
+  }
+  CoverageMappings.clear();
+  CoverageMappings.shrink_to_fit();
+
   size_t FilenamesSize = OS.str().size() - CoverageMappingSize;
   // Append extra zeroes if necessary to ensure that the size of the filenames
   // and coverage mappings is a multiple of 8.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62623: Reduce memory consumption of coverage dumps

[Vedant Kumar via Phabricator via cfe-commits]

vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1393
+  size_t CoverageMappingSize = 0;
+  for (auto  : CoverageMappings) {
+CoverageMappingSize += S.size();

serge-sans-paille wrote:
> vsk wrote:
> > It doesn't look like the CoverageMappings std::vector is needed at all. 
> > Consider moving `FilenamesAndCoverageMappings` into 
> > CoverageMappingModuleGen?
> > 
> > That should reduce the memory requirements a bit more.
> It's not possible to directly store into `FilenamesAndCoverageMappings` 
> because filenames need to be stored before all mappings. It's possible to use 
> a `std::string` instead of a `std::vector` but it means that at some point, 
> memory usage will be `| CoverageMappings| x 2` while if we empty the vector 
> elements as we push them to the stream, we should keep `|CoverageMappings|`. 
> I've updated the diff to enforce this stream behavior.
Ah, thanks for explaining.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62623



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62623: Reduce memory consumption of coverage dumps

[serge via Phabricator via cfe-commits]

serge-sans-paille added inline comments.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1393
+  size_t CoverageMappingSize = 0;
+  for (auto  : CoverageMappings) {
+CoverageMappingSize += S.size();

vsk wrote:
> It doesn't look like the CoverageMappings std::vector is needed at all. 
> Consider moving `FilenamesAndCoverageMappings` into CoverageMappingModuleGen?
> 
> That should reduce the memory requirements a bit more.
It's not possible to directly store into `FilenamesAndCoverageMappings` because 
filenames need to be stored before all mappings. It's possible to use a 
`std::string` instead of a `std::vector` but it means that at some point, 
memory usage will be `| CoverageMappings| x 2` while if we empty the vector 
elements as we push them to the stream, we should keep `|CoverageMappings|`. 
I've updated the diff to enforce this stream behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62623



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62623: Reduce memory consumption of coverage dumps

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 202896.
serge-sans-paille marked an inline comment as done.
serge-sans-paille added a comment.

Update comment + force reduced memory consumption.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62623

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1388,10 +1388,19 @@
   std::string FilenamesAndCoverageMappings;
   llvm::raw_string_ostream OS(FilenamesAndCoverageMappings);
   CoverageFilenamesSectionWriter(FilenameRefs).write(OS);
-  std::string RawCoverageMappings =
-  llvm::join(CoverageMappings.begin(), CoverageMappings.end(), "");
-  OS << RawCoverageMappings;
-  size_t CoverageMappingSize = RawCoverageMappings.size();
+
+  // Stream the content of CoverageMappings to OS while keeping
+  // memory consumption under control.
+  size_t CoverageMappingSize = 0;
+  for (auto  : CoverageMappings) {
+CoverageMappingSize += S.size();
+OS << S;
+S.clear();
+S.shrink_to_fit();
+  }
+  CoverageMappings.clear();
+  CoverageMappings.shrink_to_fit();
+
   size_t FilenamesSize = OS.str().size() - CoverageMappingSize;
   // Append extra zeroes if necessary to ensure that the size of the filenames
   // and coverage mappings is a multiple of 8.


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1388,10 +1388,19 @@
   std::string FilenamesAndCoverageMappings;
   llvm::raw_string_ostream OS(FilenamesAndCoverageMappings);
   CoverageFilenamesSectionWriter(FilenameRefs).write(OS);
-  std::string RawCoverageMappings =
-  llvm::join(CoverageMappings.begin(), CoverageMappings.end(), "");
-  OS << RawCoverageMappings;
-  size_t CoverageMappingSize = RawCoverageMappings.size();
+
+  // Stream the content of CoverageMappings to OS while keeping
+  // memory consumption under control.
+  size_t CoverageMappingSize = 0;
+  for (auto  : CoverageMappings) {
+CoverageMappingSize += S.size();
+OS << S;
+S.clear();
+S.shrink_to_fit();
+  }
+  CoverageMappings.clear();
+  CoverageMappings.shrink_to_fit();
+
   size_t FilenamesSize = OS.str().size() - CoverageMappingSize;
   // Append extra zeroes if necessary to ensure that the size of the filenames
   // and coverage mappings is a multiple of 8.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62623: Reduce memory consumption of coverage dumps

[Konrad Wilhelm Kleine via Phabricator via cfe-commits]

kwk added a comment.

In D62623#1524666 , @kwk wrote:

> In D62623#1522575 , 
> @serge-sans-paille wrote:
>
> > @kwk: looks like you're still compiling with clang-7, this patch is to be 
> > applied on the master version of LLVM/clang, then you can install it and 
> > use it to recompile llvm/lld with coverage info.
> >
> > Does it make sense to you?
>
>
> @serge-sans-paille oh yes. Absolutely. Thank you! I will try that out right 
> away.


I no longer have a crash during compilation of `ninja lldb`. Thank you so much 
@serge-sans-paille  for addressing this and helping me on IRC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62623



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62623: Reduce memory consumption of coverage dumps

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1393
+  size_t CoverageMappingSize = 0;
+  for (auto  : CoverageMappings) {
+CoverageMappingSize += S.size();

It doesn't look like the CoverageMappings std::vector is needed at all. 
Consider moving `FilenamesAndCoverageMappings` into CoverageMappingModuleGen?

That should reduce the memory requirements a bit more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62623



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62623: Reduce memory consumption of coverage dumps

[Konrad Wilhelm Kleine via Phabricator via cfe-commits]

kwk added a comment.

In D62623#1522575 , @serge-sans-paille 
wrote:

> @kwk: looks like you're still compiling with clang-7, this patch is to be 
> applied on the master version of LLVM/clang, then you can install it and use 
> it to recompile llvm/lld with coverage info.
>
> Does it make sense to you?


@serge-sans-paille oh yes. Absolutely. Thank you! I will try that out right 
away.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62623



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62623: Reduce memory consumption of coverage dumps

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

@kwk: looks like you're still compiling with clang-7, this patch is to be 
applied on the master version of LLVM/clang, then you can install it and use it 
to recompile llvm/lld with coverage info.

Does it make sense to you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62623



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62623: Reduce memory consumption of coverage dumps

[Konrad Wilhelm Kleine via Phabricator via cfe-commits]

kwk added a comment.

In D62623#1521999 , @kwk wrote:

> @serge-sans-paille testing it now.


Unfortunately, the error for me is similar (or the same) to before;

  [692/2860] Building CXX object 
lib/Target/AMDGPU/Utils/CMakeFiles/LLVMAMDGPUUtils.dir/AMDGPUBaseInfo.cpp.o
  FAILED: 
lib/Target/AMDGPU/Utils/CMakeFiles/LLVMAMDGPUUtils.dir/AMDGPUBaseInfo.cpp.o 
  /usr/bin/clang++  -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS 
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Target/AMDGPU/Utils 
-I/home/kkleine/dev/llvm-project/llvm/lib/Target/AMDGPU/Utils 
-I/home/kkleine/dev/llvm-project/llvm/lib/Target/AMDGPU -Ilib/Target/AMDGPU 
-I/usr/include/libxml2 -Iinclude -I/home/kkleine/dev/llvm-project/llvm/include 
-fPIC -fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -std=c++11 -Wall -Wextra 
-Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers 
-pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default 
-Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor 
-Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections 
-fprofile-instr-generate='/home/kkleine/llvm-builds/coverage-ninja-clang-lld/profiles/%4m.profraw'
 -fcoverage-mapping -O2 -DNDEBUG-fno-exceptions -fno-rtti -MD -MT 
lib/Target/AMDGPU/Utils/CMakeFiles/LLVMAMDGPUUtils.dir/AMDGPUBaseInfo.cpp.o -MF 
lib/Target/AMDGPU/Utils/CMakeFiles/LLVMAMDGPUUtils.dir/AMDGPUBaseInfo.cpp.o.d 
-o lib/Target/AMDGPU/Utils/CMakeFiles/LLVMAMDGPUUtils.dir/AMDGPUBaseInfo.cpp.o 
-c 
/home/kkleine/dev/llvm-project/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
  LLVM ERROR: out of memory
  Stack dump:
  0.Program arguments: /usr/bin/clang-7 -cc1 -triple 
x86_64-unknown-linux-gnu -emit-obj -disable-free -disable-llvm-verifier 
-discard-value-names -main-file-name AMDGPUBaseInfo.cpp -mrelocation-model pic 
-pic-level 2 -mthread-model posix -fmath-errno -masm-verbose 
-mconstructor-aliases -munwind-tables -fuse-init-array -target-cpu x86-64 
-dwarf-column-info -debugger-tuning=gdb -momit-leaf-frame-pointer 
-ffunction-sections -fdata-sections 
-fprofile-instrument-path=/home/kkleine/llvm-builds/coverage-ninja-clang-lld/profiles/%4m.profraw
 -fprofile-instrument=clang -fcoverage-mapping -coverage-notes-file 
/home/kkleine/llvm-builds/coverage-ninja-clang-lld/lib/Target/AMDGPU/Utils/CMakeFiles/LLVMAMDGPUUtils.dir/AMDGPUBaseInfo.cpp.gcno
 -resource-dir /usr/lib64/clang/7.0.1 -dependency-file 
lib/Target/AMDGPU/Utils/CMakeFiles/LLVMAMDGPUUtils.dir/AMDGPUBaseInfo.cpp.o.d 
-sys-header-deps -MT 
lib/Target/AMDGPU/Utils/CMakeFiles/LLVMAMDGPUUtils.dir/AMDGPUBaseInfo.cpp.o -D 
GTEST_HAS_RTTI=0 -D _GNU_SOURCE -D __STDC_CONSTANT_MACROS -D 
__STDC_FORMAT_MACROS -D __STDC_LIMIT_MACROS -I lib/Target/AMDGPU/Utils -I 
/home/kkleine/dev/llvm-project/llvm/lib/Target/AMDGPU/Utils -I 
/home/kkleine/dev/llvm-project/llvm/lib/Target/AMDGPU -I lib/Target/AMDGPU -I 
/usr/include/libxml2 -I include -I /home/kkleine/dev/llvm-project/llvm/include 
-D NDEBUG -internal-isystem 
/usr/bin/../lib/gcc/x86_64-redhat-linux/8/../../../../include/c++/8 
-internal-isystem 
/usr/bin/../lib/gcc/x86_64-redhat-linux/8/../../../../include/c++/8/x86_64-redhat-linux
 -internal-isystem 
/usr/bin/../lib/gcc/x86_64-redhat-linux/8/../../../../include/c++/8/backward 
-internal-isystem /usr/local/include -internal-isystem 
/usr/lib64/clang/7.0.1/include -internal-externc-isystem /include 
-internal-externc-isystem /usr/include -O2 -Werror=date-time 
-Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter 
-Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wno-long-long 
-Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type 
-Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -pedantic 
-std=c++11 -fdeprecated-macro -fdebug-compilation-dir 
/home/kkleine/llvm-builds/coverage-ninja-clang-lld -ferror-limit 19 
-fmessage-length 0 -fvisibility-inlines-hidden -fno-rtti -fobjc-runtime=gcc 
-fdiagnostics-show-option -fcolor-diagnostics -vectorize-loops -vectorize-slp 
-o lib/Target/AMDGPU/Utils/CMakeFiles/LLVMAMDGPUUtils.dir/AMDGPUBaseInfo.cpp.o 
-x c++ 
/home/kkleine/dev/llvm-project/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp 
-faddrsig 
  1. parser at end of file
  2.Per-file LLVM IR generation
  #0 0x7f814657b09e llvm::sys::PrintStackTrace(llvm::raw_ostream&) 
(/lib64/libLLVM-7.so+0x9b409e)
  #1 0x7f8146579554 llvm::sys::RunSignalHandlers() 
(/lib64/libLLVM-7.so+0x9b2554)
  #2 0x7f81465796d5 (/lib64/libLLVM-7.so+0x9b26d5)
  #3 0x7f8145bb8070 __restore_rt (/lib64/libpthread.so.0+0x13070)
  #4 0x7f8144b6457f __GI_raise (/lib64/libc.so.6+0x3857f)
  #5 0x7f8144b4e895 __GI_abort (/lib64/libc.so.6+0x22895)
  #6 0x7f81464fde34 llvm::report_bad_alloc_error(char const*, bool) 
(/lib64/libLLVM-7.so+0x936e34)
  #7 0x7f8144f28b34 operator new(unsigned long) 
(/lib64/libstdc++.so.6+0x97b34)
  

[PATCH] D62623: Reduce memory consumption of coverage dumps

[Konrad Wilhelm Kleine via Phabricator via cfe-commits]

kwk added a comment.

@serge-sans-paille testing it now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62623



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62623: Reduce memory consumption of coverage dumps

[serge via Phabricator via cfe-commits]

serge-sans-paille created this revision.
serge-sans-paille added reviewers: vsk, arphaman.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.
serge-sans-paille edited the summary of this revision.

Avoiding an intermediate join operation, which in turns removes the need for an
intermediate buffer that may be quite large, as showcased by

  https://bugs.llvm.org/show_bug.cgi?id=41965


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D62623

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1388,10 +1388,12 @@
   std::string FilenamesAndCoverageMappings;
   llvm::raw_string_ostream OS(FilenamesAndCoverageMappings);
   CoverageFilenamesSectionWriter(FilenameRefs).write(OS);
-  std::string RawCoverageMappings =
-  llvm::join(CoverageMappings.begin(), CoverageMappings.end(), "");
-  OS << RawCoverageMappings;
-  size_t CoverageMappingSize = RawCoverageMappings.size();
+
+  size_t CoverageMappingSize = 0;
+  for (auto  : CoverageMappings) {
+CoverageMappingSize += S.size();
+OS << std::move(S);
+  }
   size_t FilenamesSize = OS.str().size() - CoverageMappingSize;
   // Append extra zeroes if necessary to ensure that the size of the filenames
   // and coverage mappings is a multiple of 8.


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1388,10 +1388,12 @@
   std::string FilenamesAndCoverageMappings;
   llvm::raw_string_ostream OS(FilenamesAndCoverageMappings);
   CoverageFilenamesSectionWriter(FilenameRefs).write(OS);
-  std::string RawCoverageMappings =
-  llvm::join(CoverageMappings.begin(), CoverageMappings.end(), "");
-  OS << RawCoverageMappings;
-  size_t CoverageMappingSize = RawCoverageMappings.size();
+
+  size_t CoverageMappingSize = 0;
+  for (auto  : CoverageMappings) {
+CoverageMappingSize += S.size();
+OS << std::move(S);
+  }
   size_t FilenamesSize = OS.str().size() - CoverageMappingSize;
   // Append extra zeroes if necessary to ensure that the size of the filenames
   // and coverage mappings is a multiple of 8.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits