[PATCH] D44620: [XRay][profiler] Part 4: Profiler Mode Wiring

[Dean Michael Berris via Phabricator via llvm-commits]

dberris planned changes to this revision.
dberris added inline comments.


================
Comment at: compiler-rt/lib/xray/xray_profiler.cc:41
+
+struct alignas(64) ProfilingTLD {
+  FunctionCallTrie::Allocators *Allocators = nullptr;
----------------
kpw wrote:
> Can you expand the initialism to thread-local data in a comment? I always think TopLevelDomain when I see this and after jumping to definition that would help refresh my memory.
Renamed to ProfilingData instead.


================
Comment at: compiler-rt/lib/xray/xray_profiler.cc:142-143
+  auto TSC = readTSC(CPU);
+  if (ReentranceGuard)
+    return;
+  ReentranceGuard = true;
----------------
kpw wrote:
> Maybe check verbosity and Report?
Yeah, we'll need to refactor this to instead use the same reentrance guard across all the implementations (FDR and Basic). I'll add a dependency to the C++ ABI changes which has those changes.


================
Comment at: compiler-rt/lib/xray/xray_profiler.cc:189-190
+
+  // Wait a grace period to allow threads to see that we're finalizing.
+  __sanitizer::SleepForMillis(profilerFlags()->xray_profiling_grace_period_ms);
+
----------------
kpw wrote:
> Not scoped to profiling mode: This stragegy still makes me uncomfortable for cases where not much is instrumented (e.g. event tracing), but I don't have a better solution fleshed out.
> 
> It kind of feels like a cooperative scheduling problem where threads should check periodically if they're cancelled. Maybe we could have an option to add sleds that just do a finalizing check without instrumenting so that sparse instrumentation is able to respect the grace period.
Yep, we talked about this offline and we'll need to fix this across the implementations anyway. Let's fix that later.


================
Comment at: compiler-rt/lib/xray/xray_profiler.cc:198
+  // Then we force serialize the log data.
+  profileCollectorService::serialize();
+
----------------
kpw wrote:
> Is this protected from simultaneous calls to postCurrentThreadFCT if other threads take a while to see they're finalized.
serialize() has internal synchronisation, so we're relying on that synchronisation to do the right thing.


================
Comment at: compiler-rt/lib/xray/xray_profiler.cc:210-213
+  if (!__sanitizer::atomic_compare_exchange_strong(
+          &ProfilerLogStatus, &CurrentStatus,
+          XRayLogInitStatus::XRAY_LOG_INITIALIZING,
+          __sanitizer::memory_order_release)) {
----------------
kpw wrote:
> Do you have that graph of valid state transitions? I though it was OK to go from FINALIZED back to INITIALIZED without going back to UNITIALIZED.
Yeah, unfortunately it seems that we're going to need to make it so that once an implementation has flushed, it should go back to UNINITIALIZED. Either that, or we're going to have to be a bit more clever about this.


================
Comment at: compiler-rt/lib/xray/xray_profiler.cc:229-230
+    const char *ProfilerCompileFlags = profilerCompilerDefinedFlags();
+    ConfigParser.ParseString(ProfilerCompileFlags);
+    ConfigParser.ParseString(static_cast<const char *>(Options));
+    if (Verbosity())
----------------
kpw wrote:
> Can these error for bad/missing flags?
Yes, but we're really just ignoring them here -- the parser will already report if the verbosity is high enough.


================
Comment at: compiler-rt/test/xray/TestCases/Posix/profiling-multi-threaded.cc:4-5
+//
+// FIXME: Make -fxray-modes=xray-profiling part of the default?
+// RUN: %clangxx_xray -std=c++11 %s -o %t -fxray-modes=xray-profiler
+// RUN: %run %t
----------------
kpw wrote:
> Is it xray-profiler or xray-profiling? Does the flag not match the mode string in code? You assert on xray-profiling and set the mode to that in code below.
There's two parts here -- there's the mode, which in this case is a name for the implementation. We're using "profiler" to be consistent with "flight data recorder" and "basic". We could just make this "xray-profiling" all throughout, which I think would make it much simpler -- and pretend that "FDR" is "flight data recording" instead. ;)


================
Comment at: compiler-rt/test/xray/TestCases/Posix/profiling-multi-threaded.cc:8
+//
+// UNSUPPORTED: target-is-mips64,target-is-mips64el
+
----------------
kpw wrote:
> Do you need a thing to exclude windows since you're calling readtsc()
Good point. Yes, need to require Linux for now.


================
Comment at: compiler-rt/test/xray/TestCases/Posix/profiling-single-threaded.cc:47
+  assert(buffer_counter == 1);
+  assert(__xray_log_flushLog() == XRayLogFlushStatus::XRAY_LOG_FLUSHED);
+}
----------------
kpw wrote:
> Could be illustrative and increase coverage to have a test case that verifies that profiling mode can turn back on after a "round."
Good call, let me do that in the next round.


https://reviews.llvm.org/D44620