Bug 773428 - Part 1: Support dynamic profiler markers. r=ehsan

Add support for dynamic markers. All markers are copied and a signal lock is introduced to guard the stack usage. Because the marker stack is guarded by the signal lock the variables are no longer volatile.

--HG--
extra : rebase_source : 9c87347065bdff85430f53453099941895d064da

tools/profiler/TableTicker.cpp

@@ -705,6 +705,26 @@ void TableTicker::BuildJSObject(JSAObjectBuilder& b, JSCustomObject* profile)
   SetPaused(false);
 }
 
+static
+void addDynamicTag(ThreadProfile &aProfile, char aTagName, const char *aStr)
+{
+  aProfile.addTag(ProfileEntry(aTagName, ""));
+  // Add one to store the null termination
+  size_t strLen = strlen(aStr) + 1;
+  for (size_t j = 0; j < strLen;) {
+    // Store as many characters in the void* as the platform allows
+    char text[sizeof(void*)];
+    int len = sizeof(void*)/sizeof(char);
+    if (j+len >= strLen) {
+      len = strLen - j;
+    }
+    memcpy(text, &aStr[j], len);
+    j += sizeof(void*)/sizeof(char);
+    // Cast to *((void**) to pass the text data to a void*
+    aProfile.addTag(ProfileEntry('d', *((void**)(&text[0]))));
+  }
+}
+
 static
 void addProfileEntry(volatile StackEntry &entry, ThreadProfile &aProfile,
                      ProfileStack *stack, void *lastpc)
@@ -718,19 +738,7 @@ void addProfileEntry(volatile StackEntry &entry, ThreadProfile &aProfile,
     // Store the string using 1 or more 'd' (dynamic) tags
     // that will happen to the preceding tag
 
-    aProfile.addTag(ProfileEntry('c', ""));
-    // Add one to store the null termination
-    size_t strLen = strlen(sampleLabel) + 1;
-    for (size_t j = 0; j < strLen;) {
-      // Store as many characters in the void* as the platform allows
-      char text[sizeof(void*)];
-      for (size_t pos = 0; pos < sizeof(void*) && j+pos < strLen; pos++) {
-        text[pos] = sampleLabel[j+pos];
-      }
-      j += sizeof(void*)/sizeof(char);
-      // Cast to *((void**) to pass the text data to a void*
-      aProfile.addTag(ProfileEntry('d', *((void**)(&text[0]))));
-    }
+    addDynamicTag(aProfile, 'c', sampleLabel);
     if (entry.js()) {
       if (!entry.pc()) {
         // The JIT only allows the top-most entry to have a NULL pc
@@ -907,7 +915,7 @@ void TableTicker::Tick(TickSample* sample)
   // Marker(s) come before the sample
   ProfileStack* stack = mPrimaryThreadProfile.GetStack();
   for (int i = 0; stack->getMarker(i) != NULL; i++) {
-    mPrimaryThreadProfile.addTag(ProfileEntry('m', stack->getMarker(i)));
+    addDynamicTag(mPrimaryThreadProfile, 'm', stack->getMarker(i));
   }
   stack->mQueueClearMarker = true;
 

tools/profiler/sps_sampler.h

@@ -274,6 +274,10 @@ public:
 
   void addMarker(const char *aMarker)
   {
+    char* markerCopy = strdup(aMarker);
+    mSignalLock = true;
+    STORE_SEQUENCER();
+
     if (mQueueClearMarker) {
       clearMarkers();
     }
@@ -283,18 +287,24 @@ public:
     if (size_t(mMarkerPointer) == mozilla::ArrayLength(mMarkers)) {
       return; //array full, silently drop
     }
-    mMarkers[mMarkerPointer] = aMarker;
-    STORE_SEQUENCER();
+    mMarkers[mMarkerPointer] = markerCopy;
     mMarkerPointer++;
+
+    mSignalLock = false;
+    STORE_SEQUENCER();
   }
 
   // called within signal. Function must be reentrant
   const char* getMarker(int aMarkerId)
   {
-    if (mQueueClearMarker) {
-      clearMarkers();
-    }
-    if (aMarkerId < 0 ||
+    // if mSignalLock then the stack is inconsistent because it's being
+    // modified by the profiled thread. Post pone these markers
+    // for the next sample. The odds of a livelock are nearly impossible
+    // and would show up in a profile as many sample in 'addMarker' thus
+    // we ignore this scenario.
+    // if mQueueClearMarker then we've the sampler thread has already
+    // thread the markers then they are pending deletion.
+    if (mSignalLock || mQueueClearMarker || aMarkerId < 0 ||
       static_cast<mozilla::sig_safe_t>(aMarkerId) >= mMarkerPointer) {
       return NULL;
     }
@@ -304,6 +314,9 @@ public:
   // called within signal. Function must be reentrant
   void clearMarkers()
   {
+    for (mozilla::sig_safe_t i = 0; i < mMarkerPointer; i++) {
+      free(mMarkers[i]);
+    }
     mMarkerPointer = 0;
     mQueueClearMarker = false;
   }
@@ -370,11 +383,13 @@ public:
   // Keep a list of active checkpoints
   StackEntry volatile mStack[1024];
   // Keep a list of active markers to be applied to the next sample taken
-  char const * volatile mMarkers[1024];
+  char* mMarkers[1024];
  private:
   // This may exceed the length of mStack, so instead use the stackSize() method
   // to determine the number of valid samples in mStack
-  volatile mozilla::sig_safe_t mStackPointer;
+  mozilla::sig_safe_t mStackPointer;
+  // If this is set then it's not safe to read mStackPointer from the signal handler
+  volatile bool mSignalLock;
  public:
   volatile mozilla::sig_safe_t mMarkerPointer;
   // We don't want to modify _markers from within the signal so we allow
@@ -433,6 +448,12 @@ inline void mozilla_sampler_add_marker(const char *aMarker)
   if (!stack_key_initialized)
     return;
 
+  // Don't insert a marker if we're not profiling to avoid
+  // the heap copy (malloc).
+  if (!mozilla_sampler_is_active()) {
+    return;
+  }
+
   ProfileStack *stack = tlsStack.get();
   if (!stack) {
     return;