[llvm-profdata] GUIDToFuncNameMap can be static

[huangjd]

Summary

FunctionSamples::GUIDToFuncNameMap can be static instead of per
function sample. It is only used by SampleProfileLoader on a per module
basis and there's never a case (and we probably don't want such case)
that different FunctionSamples have differnet GUIDToFuncNameMap because it's
just reverse MD5 lookup!

This reduce the size of a FunctionSamples object, which has been a
bottleneck on profile loading.

[hoy]

llvm/lib/Transforms/IPO/SampleProfile.cpp

363

A static GUIDToFuncNameMap is shared across threads but here it's made to point to a module-local DS. This could be problematic. Also it cause race condition.

[huangjd]

Use thread_local

[huangjd]

llvm/lib/Transforms/IPO/SampleProfile.cpp

363

Changed it to thread_local

[hoy]

llvm/lib/Transforms/IPO/SampleProfile.cpp

362

nit: please update the comment. The update is per-module now.

Also it might be better to just move this out of the constructor into SampleProfileLoader::runOnModule. The container CurrentGUIDToFuncNameMap here is basically a ref to GUIDToFuncNameMap of SampleProfileLoader.

372

Please update the comment and mov it out to SampleProfileLoader::runOnModule too.

[wenlei]

V1 of this patch reverts things back to before D65848, and D65848 was added to fix corruption from race condition.

The current version of this patch is similar to V1 of D65848, but we removed TLS because of this comment on LLVM_THREAD_LOCAL in include/llvm/Support/Compiler.h:

/// This is essentially an extremely restricted analog to C++11's thread_local
/// support. It uses thread_local if available, falling back on gcc __thread
/// if not. __thread doesn't support many of the C++11 thread_local's
/// features. You should only use this for PODs that you can statically
/// initialize to some constant value. In almost all circumstances this is most
/// appropriate for use with a pointer, integer, or small aggregation of
/// pointers and integers.

[davidxl]

We should consider having singleton instance and use LLVM's synchronization support to guard updating. Assuming updating the map is rare so the contention should have low runtime impact.

[wenlei]

In D153486#4445610, @davidxl wrote:

We should consider having singleton instance and use LLVM's synchronization support to guard updating. Assuming updating the map is rare so the contention should have low runtime impact.

Essentially this is per-module map that needs to be accessed from FunctionSamples. For local (non-distributed) ThinLTO, each thin-backend thread will build part of the map (for symbols in that module) and access that part of the map, so write is not uncommon and contention could be common. I think TLS is the best option if it's available to use.

OTOH, what's the total size of FunctionSamples, and how significant is saving 8 byte in it?

[hoy]

In D153486#4445656, @wenlei wrote:

In D153486#4445610, @davidxl wrote:

We should consider having singleton instance and use LLVM's synchronization support to guard updating. Assuming updating the map is rare so the contention should have low runtime impact.

Essentially this is per-module map that needs to be accessed from FunctionSamples. For local (non-distributed) ThinLTO, each thin-backend thread will build part of the map (for symbols in that module) and access that part of the map, so write is not uncommon and contention could be common. I think TLS is the best option if it's available to use.

OTOH, what's the total size of FunctionSamples, and how significant is saving 8 byte in it?

TLS is now available since LLVM_ENABLE_THREADS is on by default, but on some platforms it's still off. A safe way might be to condition the current change under LLVM_ENABLE_THREADS.

[wenlei]

A safe way might be to condition the current change under LLVM_ENABLE_THREADS.

I don't think it's a good idea to sprinkle LLVM_ENABLE_THREADS in a bunch of places just for this.

[davidxl]

In D153486#4445656, @wenlei wrote:

In D153486#4445610, @davidxl wrote:

We should consider having singleton instance and use LLVM's synchronization support to guard updating. Assuming updating the map is rare so the contention should have low runtime impact.

Essentially this is per-module map that needs to be accessed from FunctionSamples. For local (non-distributed) ThinLTO, each thin-backend thread will build part of the map (for symbols in that module) and access that part of the map, so write is not uncommon and contention could be common. I think TLS is the best option if it's available to use.

Is this map initialized once and used mostly readonly? if that is the case, serializing initialization from all threads (in the order of tens, not hundreds) is no different from one static initialization done globally. Anyway, the effect needs to be measured to verify.

OTOH, what's the total size of FunctionSamples, and how significant is saving 8 byte in it?

[wenlei]

In D153486#4446003, @davidxl wrote:

In D153486#4445656, @wenlei wrote:

In D153486#4445610, @davidxl wrote:

We should consider having singleton instance and use LLVM's synchronization support to guard updating. Assuming updating the map is rare so the contention should have low runtime impact.

Essentially this is per-module map that needs to be accessed from FunctionSamples. For local (non-distributed) ThinLTO, each thin-backend thread will build part of the map (for symbols in that module) and access that part of the map, so write is not uncommon and contention could be common. I think TLS is the best option if it's available to use.

Is this map initialized once and used mostly readonly? if that is the case, serializing initialization from all threads (in the order of tens, not hundreds) is no different from one static initialization done globally. Anyway, the effect needs to be measured to verify.

Nope. The map will be written to every time a thread picks up a new module to run thin-backend. When processing a new module, name mapping for functions in that module will need to be written to the map. So we can't really front load all writes for each thread, as this is like a per-module map.

OTOH, what's the total size of FunctionSamples, and how significant is saving 8 byte in it?

Sorry for delayed response. This fell through the cracks..

llvm/include/llvm/ProfileData/SampleProf.h

for (const auto &C : getCallsiteSamples())
         MaxCount = std::max(MaxCount, F.second.getMaxCountInside());
     return MaxCount;
   }
 
   /// 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 Result = sampleprof_error::success;
-    if (!GUIDToFuncNameMap)
-      GUIDToFuncNameMap = Other.GUIDToFuncNameMap;
     if (Context.getName().empty())
       Context = Other.getContext();
     if (FunctionHash == 0) {
       // Set the function hash code for the target profile.
       FunctionHash = Other.getFunctionHash();
     } else if (FunctionHash != Other.getFunctionHash()) {
       // The two profiles coming with different valid hash codes indicates
       // either:
public:
 
   /// Translate \p Name into its original name.
   /// When profile doesn't use MD5, \p Name needs no translation.
   /// When profile uses MD5, \p Name in current FunctionSamples
   /// is actually GUID of the original function name. getFuncName will
   /// translate \p Name in current FunctionSamples into its original name
   /// by looking up in the function map GUIDToFuncNameMap.
   /// If the original name doesn't exist in the map, return empty StringRef.
-  StringRef getFuncName(StringRef Name) const {
+  static StringRef getFuncName(StringRef Name) {
     if (!UseMD5)
       return Name;
 
     assert(GUIDToFuncNameMap && "GUIDToFuncNameMap needs to be populated first");
     return GUIDToFuncNameMap->lookup(std::stoull(Name.data()));
   }
 
   /// Returns the line offset to the start line of the subprogram.
public:
   /// Whether the profile contains any ".__uniq." suffix in a name.
   static bool HasUniqSuffix;
 
   /// If this profile uses flow sensitive discriminators.
   static bool ProfileIsFS;
 
   /// GUIDToFuncNameMap saves the mapping from GUID to the symbol name, for
   /// all the function symbols defined or declared in current module.
-  DenseMap<uint64_t, StringRef> *GUIDToFuncNameMap = nullptr;
+  static DenseMap<uint64_t, StringRef> *GUIDToFuncNameMap;
 
   // Assume the input \p Name is a name coming from FunctionSamples itself.
   // If UseMD5 is true, the name is already a GUID and we
   // don't want to return the GUID of GUID.
   static uint64_t getGUID(StringRef Name) {
     return UseMD5 ? std::stoull(Name.data()) : Function::getGUID(Name);
   }
 
   // Find all the names in the current FunctionSamples including names in
   // all the inline instances and names of call targets.
   void findAllNames(DenseSet<StringRef> &NameSet) const;
 
   bool operator==(const FunctionSamples &Other) const {
-    return (GUIDToFuncNameMap == Other.GUIDToFuncNameMap ||
-            (GUIDToFuncNameMap && Other.GUIDToFuncNameMap &&
-             *GUIDToFuncNameMap == *Other.GUIDToFuncNameMap)) &&
-           FunctionHash == Other.FunctionHash && Context == Other.Context &&
+    return FunctionHash == Other.FunctionHash && Context == Other.Context &&
            TotalSamples == Other.TotalSamples &&
            TotalHeadSamples == Other.TotalHeadSamples &&
            BodySamples == Other.BodySamples &&
            CallsiteSamples == Other.CallsiteSamples;
   }
 
   bool operator!=(const FunctionSamples &Other) const {
     return !(*this == Other);

llvm/lib/ProfileData/SampleProf.cpp

 namespace llvm {
 namespace sampleprof {
 bool FunctionSamples::ProfileIsProbeBased = false;
 bool FunctionSamples::ProfileIsCS = false;
 bool FunctionSamples::ProfileIsPreInlined = false;
 bool FunctionSamples::UseMD5 = false;
 bool FunctionSamples::HasUniqSuffix = true;
 bool FunctionSamples::ProfileIsFS = false;
+DenseMap<uint64_t, StringRef> *FunctionSamples::GUIDToFuncNameMap = nullptr;
 } // namespace sampleprof
 } // namespace llvm
 
 namespace {
 
 // FIXME: This class is only here to support the transition to llvm::Error. It
 // will be removed once this transition is complete. Clients should prefer to
 // deal with the Error value directly, rather than converting to error_code.

llvm/lib/Transforms/IPO/SampleProfile.cpp

for (const auto &F : CurrentModule) {
       // we need to add the substring of function name without the suffix
       // into the GUIDToFuncNameMap.
       StringRef CanonName = FunctionSamples::getCanonicalFnName(F);
       if (CanonName != OrigName)
         CurrentGUIDToFuncNameMap.insert(
             {Function::getGUID(CanonName), CanonName});
     }
 
     // Update GUIDToFuncNameMap for each function including inlinees.
-    SetGUIDToFuncNameMapForAll(&CurrentGUIDToFuncNameMap);
+    FunctionSamples::GUIDToFuncNameMap = &CurrentGUIDToFuncNameMap;
   }
 
   ~GUIDToFuncNameMapper() {
     if (!CurrentReader.useMD5())
       return;
 
     CurrentGUIDToFuncNameMap.clear();
 
     // Reset GUIDToFuncNameMap for of each function as they're no
     // longer valid at this point.
-    SetGUIDToFuncNameMapForAll(nullptr);
+    FunctionSamples::GUIDToFuncNameMap = nullptr;
   }
 
 private:
-  void SetGUIDToFuncNameMapForAll(DenseMap<uint64_t, StringRef> *Map) {
-    std::queue<FunctionSamples *> FSToUpdate;
-    for (auto &IFS : CurrentReader.getProfiles()) {
-      FSToUpdate.push(&IFS.second);
-    }
-
-    while (!FSToUpdate.empty()) {
-      FunctionSamples *FS = FSToUpdate.front();
-      FSToUpdate.pop();
-      FS->GUIDToFuncNameMap = Map;
-      for (const auto &ICS : FS->getCallsiteSamples()) {
-        const FunctionSamplesMap &FSMap = ICS.second;
-        for (const auto &IFS : FSMap) {
-          FunctionSamples &FS = const_cast<FunctionSamples &>(IFS.second);
-          FSToUpdate.push(&FS);
-        }
-      }
-    }
-  }
-
   SampleProfileReader &CurrentReader;
   Module &CurrentModule;
   DenseMap<uint64_t, StringRef> &CurrentGUIDToFuncNameMap;
 };
 
 // Inline candidate used by iterative callsite prioritized inliner
 struct InlineCandidate {
   CallBase *CallInstr;
for (auto &BB : F) {
       bool Hot = false;
       SmallVector<CallBase *, 10> AllCandidates;
       SmallVector<CallBase *, 10> ColdCandidates;
       for (auto &I : BB) {
         const FunctionSamples *FS = nullptr;
         if (auto *CB = dyn_cast<CallBase>(&I)) {
           if (!isa<IntrinsicInst>(I)) {
             if ((FS = findCalleeFunctionSamples(*CB))) {
-              assert((!FunctionSamples::UseMD5 || FS->GUIDToFuncNameMap) &&
+              assert((!FunctionSamples::UseMD5 ||
+                      FunctionSamples::GUIDToFuncNameMap) &&
                      "GUIDToFuncNameMap has to be populated");
               AllCandidates.push_back(CB);
               if (FS->getHeadSamplesEstimate() > 0 ||
                   FunctionSamples::ProfileIsCS)
                 LocalNotInlinedCallSites.insert({CB, FS});
               if (callsiteIsHot(FS, PSI, ProfAccForSymsInList))
                 Hot = true;
               else if (shouldInlineColdCallee(*CB))