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

Keith via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 23 18:05:14 PDT 2018


kpw added a comment.

This one is more straightforward than the previous in the change. The main ideas all fall into place nicely, but I have pointed out some details that could use some attention.



================
Comment at: compiler-rt/lib/xray/xray_profiler.cc:41
+
+struct alignas(64) ProfilingTLD {
+  FunctionCallTrie::Allocators *Allocators = nullptr;
----------------
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.


================
Comment at: compiler-rt/lib/xray/xray_profiler.cc:51
+  thread_local bool UNUSED Once = [] {
+    pthread_setspecific(ProfilingKey, &TLD);
+    return false;
----------------
Don't you need pthread_create first for ProfilingKey?


================
Comment at: compiler-rt/lib/xray/xray_profiler.cc:105
+          XRayLogFlushStatus::XRAY_LOG_FLUSHING,
+          __sanitizer::memory_order_release)) {
+    if (__sanitizer::Verbosity())
----------------
Seems to me this should be memory_order_acquire_release if we want mutual exclusion of profileCollectorService::reset()  from another thread.


================
Comment at: compiler-rt/lib/xray/xray_profiler.cc:142-143
+  auto TSC = readTSC(CPU);
+  if (ReentranceGuard)
+    return;
+  ReentranceGuard = true;
----------------
Maybe check verbosity and Report?


================
Comment at: compiler-rt/lib/xray/xray_profiler.cc:156-168
+  switch (Entry) {
+  case XRayEntryType::ENTRY:
+  case XRayEntryType::LOG_ARGS_ENTRY:
+    TLD.FCT->enterFunction(FuncId, TSC);
+    break;
+  case XRayEntryType::EXIT:
+  case XRayEntryType::TAIL:
----------------
I think this should be wrapped in an "if (TLD.FCT)" block.

It's definitely an edge case, but If ProfileLogStatus is INITIALIZED, and if the atomic load happens and before the next statement a context switch and update to FINALIZING happens, then the TLD can be unitialized, but the status check won't know about the FINALIZING statement and we'll deference a null TLD.FCT.

I think there is a similar problem with moving the GetTLD() before the atomic_load for the transition from FINALIZED to INITIALIZING. You could solve it by having GetTLD return the TLD reference and status from the load.

It also might be worth documenting that InternalAlloc won't return null (as opposed to FCT allocator, so we're relying on that.


================
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);
+
----------------
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.


================
Comment at: compiler-rt/lib/xray/xray_profiler.cc:198
+  // Then we force serialize the log data.
+  profileCollectorService::serialize();
+
----------------
Is this protected from simultaneous calls to postCurrentThreadFCT if other threads take a while to see they're finalized.


================
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)) {
----------------
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.


================
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())
----------------
Can these error for bad/missing flags?


================
Comment at: compiler-rt/lib/xray/xray_profiler.cc:240
+  static bool UNUSED Once = [] {
+    pthread_key_create(&ProfilingKey, +[](void *) {
+      // This is the thread-exit handler.
----------------
Ahh. Can you just make a comment near the pthread_set_specific that INITIALIZE is responsible for calling pthread_key_create.


================
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
----------------
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.


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


================
Comment at: compiler-rt/test/xray/TestCases/Posix/profiling-multi-threaded.cc:29
+XRAY_NEVER_INSTRUMENT void process_buffer(const char *, XRayBuffer) {
+  // FIXME: Actually assert the contents of the buffer.
+  ++buffer_counter;
----------------
Yes please. ;)


================
Comment at: compiler-rt/test/xray/TestCases/Posix/profiling-single-threaded.cc: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
----------------
Similar flag confusion.


================
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);
+}
----------------
Could be illustrative and increase coverage to have a test case that verifies that profiling mode can turn back on after a "round."


https://reviews.llvm.org/D44620





More information about the llvm-commits mailing list