[PATCH] D69586: [profile] Support online merging with continuous sync mode

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: compiler-rt/lib/profile/InstrProfilingFile.c:400
+  /* By pass file truncation to allow online raw profile merging. */
+  if (lprofCurFilename.MergePoolSize)
+    return;
----------------
davidxl wrote:
> why this change?
When continuous+online-merging mode are enabled together, we must create the profile-dir so that continuous mode can be set up (create a profile to mmap()). Otherwise the profile-dir may never be created.


================
Comment at: compiler-rt/lib/profile/InstrProfilingFile.c:529
    * can succeed. Leak the file handle, as the file should stay open. */
-  setProfileFile(File);
-  int rc = writeFile(Filename);
-  if (rc)
-    PROF_ERR("Failed to write file \"%s\": %s\n", Filename, strerror(errno));
-  setProfileFile(NULL);
+  if (ProfileRequiresFullWrite) {
+    setProfileFile(File);
----------------
davidxl wrote:
> Can this code be moved up to line 499 ? Is ProfileRequiresFullWrite flag still needed?
Yes, thanks.


================
Comment at: compiler-rt/lib/profile/InstrProfilingFile.c:553
+
+unlockAndReturn:
+  if (ProfileRequiresUnlock) {
----------------
davidxl wrote:
> This can be hoisted closer to the merge handling.
In the success case, the unlock still needs to occur, so if we hoist the unlock up we'd need a 'backwards' goto. I'm not sure that would be cleaner.


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

https://reviews.llvm.org/D69586