Page MenuHomePhabricator

tunz (Choongwoo Han)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 11 2021, 5:55 AM (48 w, 4 d)

Recent Activity

Dec 2 2021

tunz added a comment to D114914: [CFG] Handle calls with funclet bundle.

Thanks for the review. Could you commit this on my behalf? I don't have commit access.

Dec 2 2021, 9:55 PM · Restricted Project
tunz updated the diff for D114914: [CFG] Handle calls with funclet bundle.

rebase

Dec 2 2021, 9:10 PM · Restricted Project

Dec 1 2021

tunz added a comment to D114760: [Sanitizer] Use CreateDirectoryA for report dirs.

Kindly ping. Can you commit this to main branch?

Dec 1 2021, 3:17 PM · Restricted Project
tunz requested review of D114914: [CFG] Handle calls with funclet bundle.
Dec 1 2021, 3:12 PM · Restricted Project

Nov 30 2021

tunz added a comment to D114760: [Sanitizer] Use CreateDirectoryA for report dirs.

Thanks for the review. Could you land it on behalf of me? I don't have commit access.

Nov 30 2021, 11:00 AM · Restricted Project

Nov 29 2021

tunz updated the summary of D114760: [Sanitizer] Use CreateDirectoryA for report dirs.
Nov 29 2021, 7:02 PM · Restricted Project
tunz updated the diff for D114760: [Sanitizer] Use CreateDirectoryA for report dirs.

fileapi.h is unnecessary

Nov 29 2021, 5:44 PM · Restricted Project
tunz updated the diff for D114760: [Sanitizer] Use CreateDirectoryA for report dirs.

Set nullptr to the second arg.

Nov 29 2021, 5:31 PM · Restricted Project
tunz requested review of D114760: [Sanitizer] Use CreateDirectoryA for report dirs.
Nov 29 2021, 5:28 PM · Restricted Project

Mar 25 2021

tunz accepted D99349: Fix: Reordering parameters in getFile and getFileOrSTDIN.
Mar 25 2021, 8:51 AM · Restricted Project
tunz updated subscribers of D99110: [Coverage] Load records immediately.

The api has changed in https://github.com/llvm/llvm-project/commit/c83cd8feef7eb8319131d968bb8c94fdc8dbb6a6 2 hours ago. now I think it should be /*IsText=*/false.
@abhina.sreeskantharajan Can you update this as well? or, I can update it today later or tomorrow. also I don't have permission to commit.

Mar 25 2021, 8:37 AM · Restricted Project

Mar 23 2021

tunz added a comment to D99110: [Coverage] Load records immediately.

Thanks. I don't have permission to commit. Could you land this on behalf of me?

Mar 23 2021, 3:01 PM · Restricted Project
tunz added a comment to D99110: [Coverage] Load records immediately.

Thanks for the review :)

Mar 23 2021, 11:09 AM · Restricted Project
tunz updated the diff for D99110: [Coverage] Load records immediately.

Addressing comments

Mar 23 2021, 11:09 AM · Restricted Project
tunz added a comment to D99110: [Coverage] Load records immediately.

In summary, there are three places allocating the memories: PGOFuncNames (string set), RequiresNullTerminator (writable file buffer), FuncRecords (string).
Since I don't fully understand the internals of profile data yet, I'm not sure if the high memory usage of PGFuncNames is the expected behavior, but anyway this change will fix not to keep those memories owned by Readers and Buffers.

Mar 23 2021, 10:14 AM · Restricted Project
tunz added a comment to D99110: [Coverage] Load records immediately.

Here is the memory profile. I couldn't take a snapshot at the peak RSS, so I took a snapshot at the first 6GB.
The followings are the top 5 heap objects and their callstack.

Mar 23 2021, 9:33 AM · Restricted Project

Mar 22 2021

tunz updated the summary of D99110: [Coverage] Load records immediately.
Mar 22 2021, 4:45 PM · Restricted Project
tunz added a comment to D99110: [Coverage] Load records immediately.
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.

I don't understand what you replaced this with. For some context: this logic is only needed for non-MachO (non-Darwin) file formats, because the use of comdats splits function records into multiple sections. See 80cd518b809dd. Looking at this some more, I admit it's possible to optimize, but I don't see how this patch does it.

Mar 22 2021, 4:43 PM · Restricted Project
tunz updated the diff for D99110: [Coverage] Load records immediately.

Addressing comments

Mar 22 2021, 4:16 PM · Restricted Project
tunz added inline comments to D99110: [Coverage] Load records immediately.
Mar 22 2021, 4:15 PM · Restricted Project
tunz added a comment to D99110: [Coverage] Load records immediately.
In 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.

Mar 22 2021, 3:00 PM · Restricted Project
tunz updated the summary of D99110: [Coverage] Load records immediately.
Mar 22 2021, 2:24 PM · Restricted Project
tunz requested review of D99110: [Coverage] Load records immediately.
Mar 22 2021, 2:17 PM · Restricted Project

Mar 2 2021

tunz added a comment to D97061: [llvm-cov] Cache file status information.

Thanks for the review! yes, I need someone to land. I don't have commit access.

Mar 2 2021, 7:31 PM · Restricted Project
tunz updated the diff for D97061: [llvm-cov] Cache file status information.

Fix typo

Mar 2 2021, 6:39 PM · Restricted Project
tunz updated the diff for D97061: [llvm-cov] Cache file status information.

Fix clang-tidy check

Mar 2 2021, 12:39 PM · Restricted Project
tunz updated the diff for D97061: [llvm-cov] Cache file status information.

Address comments

Mar 2 2021, 12:19 PM · Restricted Project
tunz added a comment to D97061: [llvm-cov] Cache file status information.

Thanks

Mar 2 2021, 11:54 AM · Restricted Project
tunz updated the diff for D97061: [llvm-cov] Cache file status information.

Use StringMap

Mar 2 2021, 11:54 AM · Restricted Project
tunz added a comment to D97061: [llvm-cov] Cache file status information.

@vsk Updated to use file status cache. It does not change the existing behavior, and performance loss is not huge (only 0.x secs for the 56 secs binary).
I didn't want to modify the LLVMSupport, so implemented the cache and rewrote the equivalent function only for llvm-cov. This still could result different outputs if some file type changes whilst running llvm-cov. But I don't think we need support that case.

Mar 2 2021, 11:40 AM · Restricted Project
tunz retitled D97061: [llvm-cov] Cache file status information from [llvm-cov] Compare path only to find the same file to [llvm-cov] Cache file status information.
Mar 2 2021, 11:35 AM · Restricted Project
tunz updated the diff for D97061: [llvm-cov] Cache file status information.

Do not update test

Mar 2 2021, 11:31 AM · Restricted Project
tunz updated the diff for D97061: [llvm-cov] Cache file status information.

Use status cache

Mar 2 2021, 11:29 AM · Restricted Project
tunz added a comment to D97061: [llvm-cov] Cache file status information.

@vsk Thanks for looking into it.

Mar 2 2021, 8:13 AM · Restricted Project
tunz updated the diff for D97061: [llvm-cov] Cache file status information.

Use a helper function

Mar 2 2021, 8:11 AM · Restricted Project

Feb 26 2021

tunz updated the diff for D97061: [llvm-cov] Cache file status information.

rebase and add doc

Feb 26 2021, 11:26 AM · Restricted Project

Feb 23 2021

tunz added a comment to D97061: [llvm-cov] Cache file status information.

@vsk @phosek Friendly ping.

Feb 23 2021, 9:46 AM · Restricted Project

Feb 19 2021

tunz updated the summary of D97061: [llvm-cov] Cache file status information.
Feb 19 2021, 9:23 AM · Restricted Project
tunz added a comment to D97061: [llvm-cov] Cache file status information.

I'm not sure about the background context about sys::fs::equivalent usage, so I added a new option for this change. I think the only difference is to follow symlinks, which doesn't look common and isn't worth keeping as a default to me.
I'd like to get some feedbacks about this change. I'll add tests and documentation if this looks good.

Feb 19 2021, 9:21 AM · Restricted Project
tunz abandoned D96696: [llvm-cov] Introduce -path-equivalence-regex.

Abandon this since https://reviews.llvm.org/D95753 addresses our issue.

Feb 19 2021, 9:16 AM · Restricted Project
tunz requested review of D97061: [llvm-cov] Cache file status information.
Feb 19 2021, 9:15 AM · Restricted Project

Feb 15 2021

tunz updated the diff for D96696: [llvm-cov] Introduce -path-equivalence-regex.

Add -path-equivalence-regex to the llvm-cov doc.

Feb 15 2021, 10:52 AM · Restricted Project
tunz updated the summary of D96696: [llvm-cov] Introduce -path-equivalence-regex.
Feb 15 2021, 10:36 AM · Restricted Project
tunz added a comment to D96696: [llvm-cov] Introduce -path-equivalence-regex.

Hi @vsk. Sorry for submitting this patch without any explanation. I was learning and testing this new environment, and was writing an email to llvm-dev.
I've just sent the email to the mailing group. https://lists.llvm.org/pipermail/llvm-dev/2021-February/148560.html

Feb 15 2021, 10:26 AM · Restricted Project
tunz updated the diff for D96696: [llvm-cov] Introduce -path-equivalence-regex.

rebase

Feb 15 2021, 8:39 AM · Restricted Project
tunz updated the diff for D96696: [llvm-cov] Introduce -path-equivalence-regex.

Add a test and do not break existing tests.

Feb 15 2021, 7:25 AM · Restricted Project
tunz requested review of D96696: [llvm-cov] Introduce -path-equivalence-regex.
Feb 15 2021, 2:11 AM · Restricted Project