[llvm-profdata] Avoid keeping reference to every files

[paulsemel]

Summary

This is definitely not the ideal solution to solve this problem.
But I think this is better than destroying my RAM.

[vsk]

Adding reviewers who are familiar with SamplePGO.

[wmi]

We had better have a test for it. It is not a strict NFC and we best verify it works as we expect -- like if we miss any place which still uses StringRef refering to string in file data buffer, we may have dangling pointer after we free the buffer.

llvm/tools/llvm-profdata/llvm-profdata.cpp

444–448

Please add some comment about why we need FunctionNames. I think it is because FunctionSamples and SampleRecord all use StringRef and the original strings refered to are in the data buffer. To remove the dependence on the data buffer, we want to use a name table to save all the name strings. The usage is not very straightforward.

[paulsemel]

In D63671#1554457, @wmi wrote:

We had better have a test for it. It is not a strict NFC and we best verify it works as we expect -- like if we miss any place which still uses StringRef refering to string in file data buffer, we may have dangling pointer after we free the buffer.

I agree with you that this is not completely NFC. However, I didn't really see what could best test this functionality (anyway, in our case, accessing dangling pointer would lead to SIGSEGV as we unmap the files).

[paulsemel]

Add comment to describe why we need this string table
Add one test case.

[paulsemel]

ping @wmi

[wmi]

In D63671#1556584, @paulsemel wrote:

Add one test case.

Thanks. If we remove the code to replace string reference from file data buffer to FunctionNames set for call targets, will the test fail?

[paulsemel]

In D63671#1563092, @wmi wrote:

In D63671#1556584, @paulsemel wrote:

Add one test case.

Thanks. If we remove the code to replace string reference from file data buffer to FunctionNames set for call targets, will the test fail?

No, because this change is kind of a NFC, which implies that I want to keep the same behaviors as previously.
To actually have a test that fails if we remove this code, we would need to have a super heavy and time consuming test, which is, imo, not what we want for a test.

Do we really need to have a tests that fails if we remove this code ?

[wmi]

In D63671#1564933, @paulsemel wrote:

In D63671#1563092, @wmi wrote:

In D63671#1556584, @paulsemel wrote:

Add one test case.

Thanks. If we remove the code to replace string reference from file data buffer to FunctionNames set for call targets, will the test fail?

No, because this change is kind of a NFC, which implies that I want to keep the same behaviors as previously.
To actually have a test that fails if we remove this code, we would need to have a super heavy and time consuming test, which is, imo, not what we want for a test.

Do we really need to have a tests that fails if we remove this code ?

Is it possible to add an option to flush the file data buffer with 0 after the file is released? The option is only enabled for testing. With the option on, even with small test we can catch some difference in merge result if the patch is incorrect.

[wmi]

In D63671#1565126, @wmi wrote:

In D63671#1564933, @paulsemel wrote:

In D63671#1563092, @wmi wrote:

In D63671#1556584, @paulsemel wrote:

Add one test case.

Thanks. If we remove the code to replace string reference from file data buffer to FunctionNames set for call targets, will the test fail?

No, because this change is kind of a NFC, which implies that I want to keep the same behaviors as previously.
To actually have a test that fails if we remove this code, we would need to have a super heavy and time consuming test, which is, imo, not what we want for a test.

Do we really need to have a tests that fails if we remove this code ?

Is it possible to add an option to flush the file data buffer with 0 after the file is released? The option is only enabled for testing. With the option on, even with small test we can catch some difference in merge result if the patch is incorrect.

I realize I wasn't clear. I mean flush the data buffer after reading and merging for the file is done and just before the file is released.

[paulsemel]

In D63671#1568997, @wmi wrote:

In D63671#1565126, @wmi wrote:

In D63671#1564933, @paulsemel wrote:

In D63671#1563092, @wmi wrote:

In D63671#1556584, @paulsemel wrote:

Add one test case.

Thanks. If we remove the code to replace string reference from file data buffer to FunctionNames set for call targets, will the test fail?

No, because this change is kind of a NFC, which implies that I want to keep the same behaviors as previously.
To actually have a test that fails if we remove this code, we would need to have a super heavy and time consuming test, which is, imo, not what we want for a test.

Do we really need to have a tests that fails if we remove this code ?

Is it possible to add an option to flush the file data buffer with 0 after the file is released? The option is only enabled for testing. With the option on, even with small test we can catch some difference in merge result if the patch is incorrect.

I realize I wasn't clear. I mean flush the data buffer after reading and merging for the file is done and just before the file is released.

I am very sorry, but I don't really understand what you want... The files are mmap'd with only READ protection, so zeroing the data isn't really an option!
Plus, we munmap the file when we destroy the object at the end of the loop, so accessing again the data would result in a segfault already... we cannot do much more here I think, but maybe I am missing smth ?

Sorry, I didn't have time to work on this the past 10 days..

[paulsemel]

ping @wmi

[wmi]

From my last reply "I assumed the input files are small since we want to have small testcase. If the input files are small, the buffer won't be allocated through mmap but through new (From getOpenFileImpl in lib/Support/MemoryBuffer.cpp)".

Do you think we can construct a testcase with small inputs so the buffer could be writable?

Plus, we munmap the file when we destroy the object at the end of the loop, so accessing again the data would result in a segfault already...

If that is the case, why you didn't see SEGV when you remove the code to replace string reference from file data buffer to FunctionNames set for call targets? I think munmap area could possibly be remapped when reading in the next file?

llvm/include/llvm/ProfileData/SampleProf.h

 
 #ifndef LLVM_PROFILEDATA_SAMPLEPROF_H
 #define LLVM_PROFILEDATA_SAMPLEPROF_H
 
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/MathExtras.h"
 #include <algorithm>
 #include <cstdint>
public:
   /// Return true if this sample record contains function calls.
   bool hasCalls() const { return !CallTargets.empty(); }
 
   uint64_t getSamples() const { return NumSamples; }
   const CallTargetMap &getCallTargets() const { return CallTargets; }
 
   /// Merge the samples in \p Other into this record.
   /// Optionally scale sample counts by \p Weight.
-  sampleprof_error merge(const SampleRecord &Other, uint64_t Weight = 1) {
+  sampleprof_error merge(const SampleRecord &Other,
+                         StringSet<> *FunctionNames = nullptr,
+                         uint64_t Weight = 1) {
     sampleprof_error Result = addSamples(Other.getSamples(), Weight);
     for (const auto &I : Other.getCallTargets()) {
-      MergeResult(Result, addCalledTarget(I.first(), I.second, Weight));
+      StringRef FName = I.first();
+      if (FunctionNames) {
+        FName = FunctionNames->insert(FName).first->first();
+      }
+      MergeResult(Result, addCalledTarget(FName, I.second, Weight));
     }
     return Result;
   }
 
   void print(raw_ostream &OS, unsigned Indent) const;
   void dump() const;
 
 private:
public:
 
   /// Return all the callsite samples collected in the body of the function.
   const CallsiteSampleMap &getCallsiteSamples() const {
     return CallsiteSamples;
   }
 
   /// Merge the samples in \p Other into this one.
   /// Optionally scale samples by \p Weight.
-  sampleprof_error merge(const FunctionSamples &Other, uint64_t Weight = 1) {
+  sampleprof_error merge(const FunctionSamples &Other,
+                         StringSet<> *FunctionNames = nullptr,
+                         uint64_t Weight = 1) {
     sampleprof_error Result = sampleprof_error::success;
     Name = Other.getName();
     MergeResult(Result, addTotalSamples(Other.getTotalSamples(), Weight));
     MergeResult(Result, addHeadSamples(Other.getHeadSamples(), Weight));
     for (const auto &I : Other.getBodySamples()) {
       const LineLocation &Loc = I.first;
       const SampleRecord &Rec = I.second;
-      MergeResult(Result, BodySamples[Loc].merge(Rec, Weight));
+      MergeResult(Result, BodySamples[Loc].merge(Rec, FunctionNames, Weight));
     }
     for (const auto &I : Other.getCallsiteSamples()) {
       const LineLocation &Loc = I.first;
       FunctionSamplesMap &FSMap = functionSamplesAt(Loc);
-      for (const auto &Rec : I.second)
-        MergeResult(Result, FSMap[Rec.first].merge(Rec.second, Weight));
+      for (const auto &Rec : I.second) {
+        if (FunctionNames) {
+          StringRef FName = Rec.second.getName();
+          FName = FunctionNames->insert(FName).first->first();
+          const_cast<FunctionSamples &>(Rec.second).setName(FName);
+        }
+        MergeResult(Result,
+                    FSMap[Rec.first].merge(Rec.second, FunctionNames, Weight));
+      }
     }
     return Result;
   }
 
   /// Recursively traverses all children, if the total sample count of the
   /// corresponding function is no less than \p Threshold, add its corresponding
   /// GUID to \p S. Also traverse the BodySamples to add hot CallTarget's GUID
   /// to \p S.

llvm/test/tools/llvm-profdata/Inputs/string-table-1.proftext

+foo:10:1
+ 1: 4
+ 2: bar:6
+bar:5:1
+ 1: 4
+ 2: verylongfunctionname:1

llvm/test/tools/llvm-profdata/Inputs/string-table-2.proftext

+verylongfunctionname:40:5
+ 1: foobar:20
+ 2: 20

llvm/test/tools/llvm-profdata/Inputs/string-table-output.proftext

+verylongfunctionname:40:5
+ 2: 20
+ 1: foobar:20
+foo:10:1
+ 1: 4
+ 2: bar:6
+bar:5:1
+ 1: 4
+ 2: verylongfunctionname:1

llvm/test/tools/llvm-profdata/string-table.test

+RUN: llvm-profdata merge -sample -text -o %t.proftext %S/Inputs/string-table-1.proftext %S/Inputs/string-table-2.proftext
+RUN: diff %t.proftext %S/Inputs/string-table-output.proftext

llvm/tools/llvm-profdata/llvm-profdata.cpp

 //===- llvm-profdata.cpp - LLVM profile data tool -------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // llvm-profdata merges .profdata files.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/ProfileData/InstrProfReader.h"
 #include "llvm/ProfileData/InstrProfWriter.h"
 #include "llvm/ProfileData/ProfileCommon.h"
 #include "llvm/ProfileData/SampleProfReader.h"
 #include "llvm/ProfileData/SampleProfWriter.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Errc.h"
static void mergeSampleProfile(const WeightedFileVector &Inputs,
   using namespace sampleprof;
   auto WriterOrErr =
       SampleProfileWriter::create(OutputFilename, FormatMap[OutputFormat]);
   if (std::error_code EC = WriterOrErr.getError())
     exitWithErrorCode(EC, OutputFilename);
 
   auto Writer = std::move(WriterOrErr.get());
   StringMap<FunctionSamples> ProfileMap;
-  SmallVector<std::unique_ptr<sampleprof::SampleProfileReader>, 5> Readers;
+
+  // This string set is used to save all function name strings.
+  // This allows not to retain in memory every ProfileReader (which
+  // implies having all the files in memory).
+  StringSet<> FunctionNames;
   LLVMContext Context;
   for (const auto &Input : Inputs) {
     auto ReaderOrErr = SampleProfileReader::create(Input.Filename, Context);
     if (std::error_code EC = ReaderOrErr.getError())
       exitWithErrorCode(EC, Input.Filename);
 
-    // We need to keep the readers around until after all the files are
-    // read so that we do not lose the function names stored in each
-    // reader's memory. The function names are needed to write out the
-    // merged profile map.
-    Readers.push_back(std::move(ReaderOrErr.get()));
-    const auto Reader = Readers.back().get();
+    const auto &Reader = ReaderOrErr.get();
     if (std::error_code EC = Reader->read())
       exitWithErrorCode(EC, Input.Filename);
 
     StringMap<FunctionSamples> &Profiles = Reader->getProfiles();
     for (StringMap<FunctionSamples>::iterator I = Profiles.begin(),
                                               E = Profiles.end();
          I != E; ++I) {
       sampleprof_error Result = sampleprof_error::success;
       FunctionSamples Remapped =
           Remapper ? remapSamples(I->second, *Remapper, Result)
                    : FunctionSamples();
       FunctionSamples &Samples = Remapper ? Remapped : I->second;
       StringRef FName = Samples.getName();
-      MergeResult(Result, ProfileMap[FName].merge(Samples, Input.Weight));
+      FName = FunctionNames.insert(FName).first->first();
+      Samples.setName(FName);
+      MergeResult(Result, ProfileMap[FName].merge(Samples, &FunctionNames,
+                                                  Input.Weight));
       if (Result != sampleprof_error::success) {
         std::error_code EC = make_error_code(Result);
         handleMergeWriterError(errorCodeToError(EC), Input.Filename, FName);
       }
     }
   }
   Writer->write(ProfileMap);
 }