[PATCH] D148417: [compiler-rt][profiling] Add an incremental buffer writing mode to libprofile

[Petr Hosek via Phabricator via llvm-commits]

phosek added a comment.

The runtime memory overhead of profile instrumentation comes from the different allocated sections: `__llvm_prf_cnts`, `__llvm_prf_data`, `__llvm_prf_vnds`, `__llvm_prf_names`; let's call all of these together //A//. When using the buffer mode, we'll need a buffer to hold all the profile data, let's call it //B//. The total memory overhead is therefore //A+B//. This change replaces //B// with a fixed size buffer //k//, so the memory overhead is going to decrease to //A+k//.

My concern is that this change adds a lot of complexity to the profile runtime, which already suffers from poor code quality and is in a dire need of a substantial rewrite/refactoring <https://discourse.llvm.org/t/using-c-and-sanitizer-common-in-profile-runtime/58629>, but the impact is somewhat limited because this change does nothing to reduce the size of //A//.

Have you considered alternatives such as using instrumentation function groups <https://clang.llvm.org/docs/UsersManual.html#instrument-function-groups>? For example, using `-fprofile-function-groups=4` would reduce the memory overhead to //(A+B)/4// which should be strictly better than //A+k//. Would that work for your use case?

Another direction we discussed for reducing the memory overhead is making `__llvm_prf_data`, `__llvm_prf_vnds`, `__llvm_prf_names` non-allocated. Our observation is that the only thing we really need in the profile are the header and the counters, everything else can be recovered from the binary (which becomes easier if the profile has binary ID and you can use debuginfod <https://reviews.llvm.org/D136702> to retrieve the binary). That's direction we'd like to pursue to support profiling in embedded environments.



================
Comment at: compiler-rt/lib/profile/InstrProfilingBuffer.c:19
+/// Global state for writing incrementally.
+static IncrementalProfileWriterState GlobalIncrementalProfileWriterState;
+
----------------
This struct is fairly large and would be always allocated, even if we aren't using the incremental mode unless it can be GC'ed by the linker (which requires compiling with GC enabled which isn't enabled by default on all platforms).

Could we move all the symbols related to the incremental mode into a separate TU so if the incremental mode isn't being used, those symbols won't be pulled into the link in in the first place?


================
Comment at: compiler-rt/lib/profile/InstrProfilingBuffer.c:278-281
+  if (initIncrementalProfileWriterState(&GlobalIncrementalProfileWriterState)) {
+    PROF_ERR_INTERNAL("%s\n", "Failed to initialize global writer state.");
+    return 1;
+  }
----------------
I think we might need to rethink the error reporting. The callees will already print their own error message, but so will the callers, so we'll end up with reporting the error multiple times.

I think we should either omit the error message here, and rely on callees to do the error reporting, or change the callees to return a return code and do all the error reporting here (but not both).


================
Comment at: compiler-rt/lib/profile/InstrProfilingWriter.c:314
+COMPILER_RT_VISIBILITY int
+lprofWriteDataImpl(ProfDataWriter *Writer, const __llvm_profile_data *DataBegin,
+                   const __llvm_profile_data *DataEnd,
----------------
Would it be possible to split this change and land it separately?


================
Comment at: compiler-rt/lib/profile/InstrProfilingWriter.c:336
+  /* Create the header. */
+  __llvm_profile_header Header = createHeader();
+
----------------
Since this is C and not C++, do you know if all compilers supported by compiler-rt perform RVO in C? It might be safer to use an output parameter here to avoid a copy.


================
Comment at: compiler-rt/lib/profile/InstrProfilingWriter.c:392
+    return 0;
+  if (SrcBufferIdxToPopulate >= NUM_PROFILING_SECTIONS_PLUS_ONE) {
+    PROF_ERR_INTERNAL("%s\n", "WriterState has too many source buffers; "
----------------
Could this be an assert?


================
Comment at: compiler-rt/lib/profile/InstrProfilingWriter.c:397
+  }
+  if (!SrcBufferData && !IsPadding) {
+    PROF_ERR_INTERNAL("%s\n", "Non-padding source buffer data; "
----------------
Could this be an assert?


================
Comment at: compiler-rt/lib/profile/InstrProfilingWriter.c:431
+        "%s\n",
+        "Binary ID writing is not yet supported in incremental writing mode");
+    return 1;
----------------
Even though Binary ID is only supported for ELF, we have a plan for how to support it for COFF and Mach-O as well, it just hasn't been implemented yet.

Do you have an idea for how we could support Binary ID with incremental mode? I'm a little wary about introducing features that are mutually incompatible without some long term plan for resolving those incompatibilities.


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

https://reviews.llvm.org/D148417