Bug 1503251 - Keep payload values in the dictionary if they are still alive. r=djvj
authorDenis Palmeiro <dpalmeiro@mozilla.com>
Fri, 07 Dec 2018 17:42:39 +0000
changeset 508843 ec925f7dac88abd54ba19d4b317b7b3699ecce1f
parent 508842 68faf9528d57faadba69b18e883a3d291a504b5e
child 508844 6f37c64ae423329d0f4d41306e7481ebbbf34ade
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdjvj
bugs1503251
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1503251 - Keep payload values in the dictionary if they are still alive. r=djvj If the JS::ResetTraceLogger() routine is called, then keep track of existing event payloads and keep their existence alive instead of clearing them. This data can still be referred to in another profiler session and can therefore cause to dangling pointers if we free them. Differential Revision: https://phabricator.services.mozilla.com/D10277
js/src/vm/TraceLogging.cpp
js/src/vm/TraceLogging.h
--- a/js/src/vm/TraceLogging.cpp
+++ b/js/src/vm/TraceLogging.cpp
@@ -455,24 +455,28 @@ TraceLoggerEventPayload* TraceLoggerThre
     }
     if (!dictionaryData.append(std::move(str))) {
       return nullptr;
     }
 
     nextDictionaryId++;
   }
 
-  uint32_t textId = nextTextId;
+  // Look for a free entry, as some textId's may
+  // already be taken from previous profiling sessions.
+  while (textIdPayloads.has(nextTextId)) {
+    nextTextId++;
+  }
 
-  auto* payload = js_new<TraceLoggerEventPayload>(textId, dictId);
+  auto* payload = js_new<TraceLoggerEventPayload>(nextTextId, dictId);
   if (!payload) {
     return nullptr;
   }
 
-  if (!textIdPayloads.putNew(textId, payload)) {
+  if (!textIdPayloads.putNew(nextTextId, payload)) {
     js_delete(payload);
     return nullptr;
   }
 
   payload->use();
 
   nextTextId++;
 
@@ -971,43 +975,82 @@ void TraceLoggerThread::log(uint32_t id)
 
   mozilla::TimeStamp time = mozilla::TimeStamp::Now();
 
   EventEntry& entry = events.pushUninitialized();
   entry.time = time;
   entry.textId = id;
 }
 
-void TraceLoggerThreadState::clear() {
-  LockGuard<Mutex> guard(lock);
-  for (TraceLoggerThread* logger : threadLoggers) {
-    logger->clear();
-  }
+bool TraceLoggerThreadState::remapDictionaryEntries(
+    mozilla::Vector<UniqueChars, 0, SystemAllocPolicy>* newDictionary,
+    uint32_t* newNextDictionaryId) {
+  MOZ_ASSERT(newNextDictionaryId != nullptr && newDictionary != nullptr);
+
+  typedef HashMap<uint32_t, uint32_t, DefaultHasher<uint32_t>,
+                  SystemAllocPolicy>
+      DictionaryMap;
+  DictionaryMap dictionaryMap;
 
   // Clear all payloads that are not currently used.  There may be some events
-  // that still hold a pointer to a payload.  Restarting the profiler may add
-  // this event to the new events array and so we need to maintain it's
-  // existence.
+  // that still hold a pointer to a payload.  Restarting the profiler may reuse
+  // the exact same event as a previous session if it's still alive so we need
+  // to maintain it's existence.
   for (TextIdToPayloadMap::Enum e(textIdPayloads); !e.empty(); e.popFront()) {
     if (e.front().value()->uses() == 0) {
       js_delete(e.front().value());
       e.removeFront();
+    } else {
+      TraceLoggerEventPayload* payload = e.front().value();
+      uint32_t dictId = payload->dictionaryId();
+
+      if (dictionaryMap.has(dictId)) {
+        DictionaryMap::Ptr mapPointer = dictionaryMap.lookup(dictId);
+        MOZ_ASSERT(mapPointer);
+        payload->setDictionaryId(mapPointer->value());
+      } else {
+        if (!newDictionary->append(std::move(dictionaryData[dictId]))) {
+          return false;
+        }
+        payload->setDictionaryId(*newNextDictionaryId);
+
+        if (!dictionaryMap.putNew(dictId, *newNextDictionaryId)) {
+          return false;
+        }
+
+        (*newNextDictionaryId)++;
+      }
     }
   }
 
-  // Clear and free any data used for the string dictionary.
-  for (auto range = dictionaryData.all(); !range.empty(); range.popFront()) {
-    range.front().reset();
+  return true;
+}
+
+void TraceLoggerThreadState::clear() {
+  LockGuard<Mutex> guard(lock);
+
+  uint32_t newNextDictionaryId = 0;
+  mozilla::Vector<UniqueChars, 0, SystemAllocPolicy> newDictionary;
+  if (remapDictionaryEntries(&newDictionary, &newNextDictionaryId)) {
+    // Clear and free any data used for the string dictionary.
+    for (auto range = dictionaryData.all(); !range.empty(); range.popFront()) {
+      range.front().reset();
+    }
+    dictionaryData.clearAndFree();
+    dictionaryData = std::move(newDictionary);
+
+    payloadDictionary.clearAndCompact();
+
+    nextTextId = TraceLogger_Last;
+    nextDictionaryId = newNextDictionaryId;
   }
 
-  dictionaryData.clearAndFree();
-  payloadDictionary.clearAndCompact();
-
-  nextTextId = TraceLogger_Last;
-  nextDictionaryId = 0;
+  for (TraceLoggerThread* logger : threadLoggers) {
+    logger->clear();
+  }
 }
 
 void TraceLoggerThread::clear() {
   if (graph.get()) {
     graph.reset();
   }
 
   graph = nullptr;
--- a/js/src/vm/TraceLogging.h
+++ b/js/src/vm/TraceLogging.h
@@ -195,16 +195,17 @@ class TraceLoggerEventPayload {
 
   void setLine(uint32_t line) { line_ = mozilla::Some(line); }
   void setColumn(uint32_t col) { col_ = mozilla::Some(col); }
 
   mozilla::Maybe<uint32_t> line() { return line_; }
   mozilla::Maybe<uint32_t> column() { return col_; }
   uint32_t textId() { return textId_; }
   uint32_t dictionaryId() { return dictionaryId_; }
+  void setDictionaryId(uint32_t dictId) { dictionaryId_ = dictId; }
   uint32_t uses() { return uses_; }
 
   // Payloads may have their use count change at any time, *except* the count
   // can only go from zero to non-zero while the thread state lock is held.
   // This should only happen under getOrCreateEventPayload below, and avoids
   // races with purgeUnusedPayloads.
   void use() {
     MOZ_ASSERT_IF(!uses_, CurrentThreadOwnsTraceLoggerThreadStateLock());
@@ -421,16 +422,19 @@ class TraceLoggerThreadState {
   bool init();
   ~TraceLoggerThreadState();
 
   void enableDefaultLogging();
   void enableIonLogging();
   void enableFrontendLogging();
 
   void clear();
+  bool remapDictionaryEntries(
+      mozilla::Vector<UniqueChars, 0, SystemAllocPolicy>* newDictionary,
+      uint32_t* newNextDictionaryId);
 
   TraceLoggerThread* forCurrentThread(JSContext* cx);
   void destroyLogger(TraceLoggerThread* logger);
 
   bool isTextIdEnabled(uint32_t textId) {
     if (textId < TraceLogger_Last) {
       return enabledTextIds[textId];
     }