[profile] in-process profile merging support Part-3

[davidxl]

Summary

With this patch, the merging feature will be fully functional.

There are two way to turn on profile merging: command line and environment variable. They both share the same mechanism %[0-9]m specifiler in the name pattern. The numeric modifier in the %m specifier specifies the size of the shared profile file pool size. By default %m is equivalent to %1m which means all processes from the same binary share one single profile file.

Example. Do an instrumented clang build with option -fprofile-instr-generate=/tmp/clang_raw_profile_%m and run clang test or do a clang bootstrap, all instrumented binaries such as clang, opt, llc, llvm-profdata etc will dump profile data into its own profile data file.

Running instrumented binary with LLVM_PROFILE_FILE=..._%m will have the same effect.

[vsk]

Awesome! Some comments/questions inline --

lib/profile/InstrProfilingFile.c

99

Possible dropped error here

102

Ditto

104

Could we skip these two checks by moving the call to __llvm_profile_check_compatibility here?

110

0 -> NULL

112

MAP_FAILED instead of -1?

149

I think any successful call to doProfileMerging ought to be followed by COMPILER_RT_FTRUNCATE. Sink it into the function?

155

Ditto, fseek error

245

Should this check be: if (MergingEnabled && (FilenamePat[I] == 'm' || FilenamePat[I + 1] == 'm'))? Why check whether the character after the 'm' is nil?

280

Could you add a comment explaining that '24' comes from lprofGetLoadModuleSignature? In fact, it might be better to hide this detail in a helper function in InstrProfilingMerge.c.

326

Can this predicate be shared with parseFilenamePattern? I'd prefer not to have it duplicated.

test/profile/instrprof-basic.c

18

Is the lprofGetLoadModuleSignature code path covered?

[davidxl]

lib/profile/InstrProfilingFile.c

104

yes

149

doProfileMerge just merges profile into memory, and the wrapper OpenFileForMerge does the rest to prepare for writing (including truncating). Another reason it is here is that doProfileMerge has multiple 'return 0' so extracting truncation code out is cleaner.

245

The second error condition is that %m specifier is not at the end of the pat. This restriction is for convenience. I don't see a reason to support %[0-9]m in arbitrary locations.

test/profile/instrprof-basic.c

18

The plan is to have basic test coverage for now. More tests will be added as follow up as things stablizes.

[davidxl]

Addressed vsk's comments.

[vsk]

lib/profile/InstrProfilingFile.c

149

Ok

238

I think "binary-%m.profraw" could be a common pattern; moreover I don't see a reason to not support this. Would removing this restriction require adding more code to getCurFilenameLength()/getCurFilename()?

test/profile/instrprof-basic.c

18

Ok

[davidxl]

Relax the constraint that %m can only be at the end of the pattern. It can now appear anywhere.

[vsk]

LGTM with one nit.

lib/profile/InstrProfilingFile.c

34

I don't think this is used anywhere, and it might be a bit cumbersome. Delete?

[silvas]

Looks like Vedant covered most of the things I wanted to mention. LGTM with a couple nits.

lib/profile/InstrProfilingFile.c

226

Naively, this seems like it may read off the end. A comment explaining why not would probably be helpful.

296

typo: ty should be by

lib/profile/InstrProfilingInternal.h

160

I think you meant "executable" instead of "executation".

lib/profile/InstrProfilingMerge.c

24

I think it is more accurate to say "a module signature" instead of "the module signature". Ideally we would e.g. hash all the names or something else a bit more robust (but I think that just the sizes will be fine for now).

[davidxl]

lib/profile/InstrProfilingInternal.h

160

Fixed.

lib/profile/InstrProfilingFile.c

 /* For _alloca. */
 #include <malloc.h>
 #endif
+#if defined(_WIN32)
+#include "WindowsMMap.h"
+/* For _chsize_s */
+#include <io.h>
+#else
+#include <sys/file.h>
+#include <sys/mman.h>
+#include <unistd.h>
+#if defined(__linux__)
+#include <sys/types.h>
+#endif
+#endif
 
+#define PROFILE_POOL_MAX_SIZE 9
 #define MAX_PID_SIZE 16
 /* Data structure holding the result of parsed filename pattern.  */
 typedef struct lprofFilename {

   char Hostname[COMPILER_RT_MAX_HOSTLEN];
   unsigned NumPids;
   unsigned NumHosts;
+  /* When in-process merging is enabled, this parameter specifies
+   * the total number of profile data files shared by all the processes
+   * spawned from the same binary. By default the value is 1. If merging
+   * is not enabled, its value should be 0. This parameter is specified
+   * by the %[0-9]m specifier. For instance %2m enables merging using
+   * 2 profile data files. %1m is equivalent to %m. Also %m specifier
+   * can only appear once at the end of the name pattern. */
+  unsigned MergePoolSize;
 } lprofFilename;
 
-lprofFilename lprofCurFilename = {0, {0}, {0}, 0, 0};
+lprofFilename lprofCurFilename = {0, {0}, {0}, 0, 0, 0};
 
 int getpid(void);
 static int getCurFilenameLength();
 static const char *getCurFilename(char *FilenameBuf);
+static unsigned doMerging() { return lprofCurFilename.MergePoolSize; }
 
 /* Return 1 if there is an error, otherwise return  0.  */
 static uint32_t fileWriter(ProfDataIOVec *IOVecs, uint32_t NumIOVecs,

   }
 }
 
+/* Read profile data in \c ProfileFile and merge with in-memory
+   profile counters. Returns -1 if there is fatal error, otheriwse
+   0 is returned.
+*/
+static int doProfileMerging(FILE *ProfileFile) {
+  uint64_t ProfileFileSize;
+  char *ProfileBuffer;
+
+  if (fseek(ProfileFile, 0L, SEEK_END) == -1) {
+    PROF_ERR("Unable to merge profile data, unable to get size: %s\n",
+             strerror(errno));
+    return -1;
+  }
+  ProfileFileSize = ftell(ProfileFile);
+
+  /* Restore file offset.  */
+  if (fseek(ProfileFile, 0L, SEEK_SET) == -1) {
+    PROF_ERR("Unable to merge profile data, unable to rewind: %s\n",
+             strerror(errno));
+    return -1;
+  }
+
+  /* Nothing to merge.  */
+  if (ProfileFileSize < sizeof(__llvm_profile_header)) {
+    if (ProfileFileSize)
+      PROF_WARN("Unable to merge profile data: %s\n",
+                "source profile file is too small.");
+    return 0;
+  }
+
+  ProfileBuffer = mmap(NULL, ProfileFileSize, PROT_READ, MAP_SHARED | MAP_FILE,
+                       fileno(ProfileFile), 0);
+  if (ProfileBuffer == MAP_FAILED) {
+    PROF_ERR("Unable to merge profile data, mmap failed: %s\n",
+             strerror(errno));
+    return -1;
+  }
+
+  if (__llvm_profile_check_compatibility(ProfileBuffer, ProfileFileSize)) {
+    (void)munmap(ProfileBuffer, ProfileFileSize);
+    PROF_WARN("Unable to merge profile data: %s\n",
+              "source profile file is not compatible.");
+    return 0;
+  }
+
+  /* Now start merging */
+  __llvm_profile_merge_from_buffer(ProfileBuffer, ProfileFileSize);
+  (void)munmap(ProfileBuffer, ProfileFileSize);
+
+  return 0;
+}
+
+/* Open the profile data for merging. It opens the file in r+b mode with
+ * file locking.  If the file has content which is compatible with the
+ * current process, it also reads in the profile data in the file and merge
+ * it with in-memory counters. After the profile data is merged in memory,
+ * the original profile data is truncated and gets ready for the profile
+ * dumper. With profile merging enabled, each executable as well as any of
+ * its instrumented shared libraries dump profile data into their own data file.
+*/
+static FILE *openFileForMerging(const char *ProfileFileName) {
+  FILE *ProfileFile;
+  int rc;
+
+  ProfileFile = lprofOpenFileEx(ProfileFileName);
+  if (!ProfileFile)
+    return NULL;
+
+  rc = doProfileMerging(ProfileFile);
+  if (rc || COMPILER_RT_FTRUNCATE(ProfileFile, 0L) ||
+      fseek(ProfileFile, 0L, SEEK_SET) == -1) {
+    PROF_ERR("Profile Merging of file %s failed: %s\n", ProfileFileName,
+             strerror(errno));
+    fclose(ProfileFile);
+    return NULL;
+  }
+  fseek(ProfileFile, 0L, SEEK_SET);
+  return ProfileFile;
+}
+
 /* Write profile data to file \c OutputName.  */
 static int writeFile(const char *OutputName) {
   int RetVal;
   FILE *OutputFile;
 
-  /* Append to the file to support profiling multiple shared objects. */
+  if (!doMerging())
     OutputFile = fopen(OutputName, "ab");
+  else
+    OutputFile = openFileForMerging(OutputName);
+
   if (!OutputFile)
     return -1;
 

   lprofCurFilename.FilenamePat = "default.profraw";
 }
 
-/* Parses the pattern string \p FilenamePat and store the result to
- * lprofcurFilename structure. */
+static int containsMergeSpecifier(const char *FilenamePat, int I) {
+  return (FilenamePat[I] == 'm' ||
+          (FilenamePat[I] >= '1' && FilenamePat[I] <= '9' &&
+           FilenamePat[I + 1] == 'm'));
+}
 
+/* Parses the pattern string \p FilenamePat and stores the result to
+ * lprofcurFilename structure. */
 static int parseFilenamePattern(const char *FilenamePat) {
   int NumPids = 0, NumHosts = 0, I;
   char *PidChars = &lprofCurFilename.PidChars[0];
   char *Hostname = &lprofCurFilename.Hostname[0];
+  int MergingEnabled = 0;
 
   lprofCurFilename.FilenamePat = FilenamePat;
   /* Check the filename for "%p", which indicates a pid-substitution. */

                 FilenamePat);
             return -1;
           }
+      } else if (containsMergeSpecifier(FilenamePat, I)) {
+        if (MergingEnabled || (FilenamePat[I] == 'm' && FilenamePat[I + 1]) ||
+            (FilenamePat[I + 1] == 'm' && FilenamePat[I + 2])) {
+          PROF_WARN(
+              "%%m specifier can only be specified once at the end of %s.\n",
+              FilenamePat);
+          return -1;
+        }
+        MergingEnabled = 1;
+        if (FilenamePat[I] == 'm')
+          lprofCurFilename.MergePoolSize = 1;
+        else
+          lprofCurFilename.MergePoolSize = FilenamePat[I] - '0';
       }
     }
 

   NewFile =
       !OldFilenamePat || (strcmp(OldFilenamePat, lprofCurFilename.FilenamePat));
 
-  if (NewFile)
+  if (NewFile && !lprofCurFilename.MergePoolSize)
     truncateCurrentFile();
 }
 
 /* Return buffer length that is required to store the current profile
  * filename with PID and hostname substitutions. */
+/* The length to hold uint64_t followed ty 2 digit pool id including '_' */
+#define SIGLEN 24
 static int getCurFilenameLength() {
+  int Len;
   if (!lprofCurFilename.FilenamePat || !lprofCurFilename.FilenamePat[0])
     return 0;
 
-  if (!(lprofCurFilename.NumPids || lprofCurFilename.NumHosts))
+  if (!(lprofCurFilename.NumPids || lprofCurFilename.NumHosts ||
+        lprofCurFilename.MergePoolSize))
     return strlen(lprofCurFilename.FilenamePat);
 
-  return strlen(lprofCurFilename.FilenamePat) +
-         lprofCurFilename.NumPids * (strlen(lprofCurFilename.PidChars) - 2) +
-         lprofCurFilename.NumHosts * (strlen(lprofCurFilename.Hostname) - 2);
+  Len = strlen(lprofCurFilename.FilenamePat) +
+  if (lprofCurFilename.MergePoolSize)
+    Len += SIGLEN;
+  return Len;
 }
 
 /* Return the pointer to the current profile file name (after substituting

   if (!lprofCurFilename.FilenamePat || !lprofCurFilename.FilenamePat[0])
     return 0;
 
-  if (!(lprofCurFilename.NumPids || lprofCurFilename.NumHosts))
+  if (!(lprofCurFilename.NumPids || lprofCurFilename.NumHosts ||
+        lprofCurFilename.MergePoolSize))
     return lprofCurFilename.FilenamePat;
 
   PidLength = strlen(lprofCurFilename.PidChars);

       } else if (FilenamePat[I] == 'h') {
         memcpy(FilenameBuf + J, lprofCurFilename.Hostname, HostNameLength);
         J += HostNameLength;
+      } else if (containsMergeSpecifier(FilenamePat, I)) {
+        char LoadModuleSignature[SIGLEN];
+        int ProfilePoolId = getpid() % lprofCurFilename.MergePoolSize;
+        snprintf(LoadModuleSignature, SIGLEN, "%" PRIu64 "_%d",
+                 lprofGetLoadModuleSignature(), ProfilePoolId);
+        memcpy(FilenameBuf + J, LoadModuleSignature, SIGLEN);
+        J += SIGLEN;
       }
       /* Drop any unknown substitutions. */
     } else

lib/profile/InstrProfilingInternal.h

 void lprofSetMaxValsPerSite(uint32_t MaxVals);
 void lprofSetupValueProfiler();
 
+/* Return the profile header 'signature' value associated with the current
+ * executation or shared library. The signature value can be used to for
+ * a profile name that is unique to this load module so that it does not
+ * collide with profiles from other binaries. It also allows shared libraries
+ * to dump merged profile data into its own profile file. */
+uint64_t lprofGetLoadModuleSignature();
+
 COMPILER_RT_VISIBILITY extern char *(*GetEnvHook)(const char *);
 COMPILER_RT_VISIBILITY extern void (*FreeHook)(void *);
 COMPILER_RT_VISIBILITY extern uint8_t *DynamicBufferIOBuffer;

lib/profile/InstrProfilingMerge.c

 
 COMPILER_RT_WEAK void (*VPMergeHook)(ValueProfData *,
                                      __llvm_profile_data *) = NULL;
+COMPILER_RT_VISIBILITY
+uint64_t lprofGetLoadModuleSignature() {
+  /* A very fast way to compute the module signature.  */
+  uint64_t CounterSize = (uint64_t)(__llvm_profile_end_counters() -
+                                    __llvm_profile_begin_counters());
+  uint64_t DataSize = __llvm_profile_get_data_size(__llvm_profile_begin_data(),
+                                                   __llvm_profile_end_data());
+  uint64_t NamesSize =
+      (uint64_t)(__llvm_profile_end_names() - __llvm_profile_begin_names());
+  uint64_t NumVnodes =
+      (uint64_t)(__llvm_profile_end_vnodes() - __llvm_profile_begin_vnodes());
+  const __llvm_profile_data *FirstD = __llvm_profile_begin_data();
+
+  return (NamesSize << 40) + (CounterSize << 30) + (DataSize << 20) +
+         (NumVnodes << 10) + (DataSize > 0 ? FirstD->NameRef : 0);
+}
 
 /* Returns 1 if profile is not structurally compatible.  */
 COMPILER_RT_VISIBILITY

       (__llvm_profile_data *)(ProfileData + sizeof(__llvm_profile_header));
   SrcDataEnd = SrcDataStart + Header->DataSize;
 
+  if (ProfileSize < sizeof(__llvm_profile_header))
+    return 1;
+
   /* Check the header first.  */
   if (Header->Magic != __llvm_profile_get_magic() ||
       Header->Version != __llvm_profile_get_version() ||

lib/profile/InstrProfilingPort.h

 #define COMPILER_RT_ALIGNAS(x) __declspec(align(x))
 #define COMPILER_RT_VISIBILITY
 #define COMPILER_RT_WEAK __declspec(selectany)
+/* Need to include <windows.h> */
 #define COMPILER_RT_ALLOCA _alloca
+/* Need to include <stdio.h> and <io.h> */
+#define COMPILER_RT_FTRUNCATE(f,l) _chsize(_fileno(f),l)
 #elif __GNUC__
 #define COMPILER_RT_ALIGNAS(x) __attribute__((aligned(x)))
 #define COMPILER_RT_VISIBILITY __attribute__((visibility("hidden")))
 #define COMPILER_RT_WEAK __attribute__((weak))
 #define COMPILER_RT_ALLOCA __builtin_alloca
+#define COMPILER_RT_FTRUNCATE(f,l) ftruncate(fileno(f),l)
 #endif
 
 #if defined(__APPLE__)

test/profile/instrprof-basic.c

 // RUN: %clang_profgen -o %t -O3 %s
 // RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
 // RUN: llvm-profdata merge -o %t.profdata %t.profraw
-// RUN: %clang_profuse=%t.profdata -o - -S -emit-llvm %s | FileCheck %s
+// RUN: %clang_profuse=%t.profdata -o - -S -emit-llvm %s | FileCheck %s --check-prefix=COMMON --check-prefix=ORIG
+//
+// RUN: rm -f %t.profraw_e_*
+// RUN: env LLVM_PROFILE_FILE=%t.profraw_e_%1m %run %t
+// RUN: env LLVM_PROFILE_FILE=%t.profraw_e_%1m %run %t
+// RUN: llvm-profdata merge -o %t.em.profdata %t.profraw_e_*
+// RUN: %clang_profuse=%t.em.profdata -o - -S -emit-llvm %s | FileCheck %s --check-prefix=COMMON --check-prefix=MERGE
+//
+// RUN: %clang -o %t.merge -fprofile-instr-generate=%t.profraw_%m -O3 %s
+// RUN: rm -f %t.profraw_*
+// RUN: %run %t.merge
+// RUN: %run %t.merge
+// RUN: llvm-profdata merge -o %t.m.profdata %t.profraw_*
+// RUN: %clang_profuse=%t.m.profdata -o - -S -emit-llvm %s | FileCheck %s --check-prefix=COMMON --check-prefix=MERGE
 
 int begin(int i) {
-  // CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !prof ![[PD1:[0-9]+]]
+  // COMMON: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !prof ![[PD1:[0-9]+]]
   if (i)
     return 0;
   return 1;
 }
 
 int end(int i) {
-  // CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !prof ![[PD2:[0-9]+]]
+  // COMMON: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !prof ![[PD2:[0-9]+]]
   if (i)
     return 0;
   return 1;

   begin(0);
   end(1);
 
-  // CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !prof ![[PD2:[0-9]+]]
+  // COMMON: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !prof ![[PD2:[0-9]+]]
   if (argc)
     return 0;
   return 1;
 }
 
-// CHECK: ![[PD1]] = !{!"branch_weights", i32 1, i32 2}
-// CHECK: ![[PD2]] = !{!"branch_weights", i32 2, i32 1}
+// ORIG: ![[PD1]] = !{!"branch_weights", i32 1, i32 2}
+// ORIG: ![[PD2]] = !{!"branch_weights", i32 2, i32 1}
+// MERGE: ![[PD1]] = !{!"branch_weights", i32 1, i32 3}
+// MERGE: ![[PD2]] = !{!"branch_weights", i32 3, i32 1}