Bug 1436179: Lazily grow the ProfileEntryStorage. r=mstange draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 18 Mar 2018 16:58:44 +0100
changeset 769854 57b476d4fa77e28d38fc11c423f00db0ba063eb5
parent 769806 6cffa8738ca53d246a39536d18a0c19b8c602e37
child 769855 03538a2496fc87ec88d2bfae2c583e88e835b992
push id103235
push userbmo:emilio@crisal.io
push dateTue, 20 Mar 2018 08:48:29 +0000
reviewersmstange
bugs1436179
milestone61.0a1
Bug 1436179: Lazily grow the ProfileEntryStorage. r=mstange Beware of me not being terribly familiar with the threading setup for the profiler. IIUC, we have exclusive access to the data in there, except for `entries` and `stackPointer`, which are read from the profiler thread. If so, I think this patch should be fine, even if the current thread is interrupted during the growing of the buffer. I've kept the max number of entries intact, though I guess it can go away now. Also the initial capacity of 50 is completely arbitrary, and probably should be changed to something like, let's say, 512? Your call. MozReview-Commit-ID: BEGP1ykl4S
js/public/ProfilingStack.h
--- a/js/public/ProfilingStack.h
+++ b/js/public/ProfilingStack.h
@@ -2,16 +2,19 @@
  * vim: set ts=8 sts=4 et sw=4 tw=99:
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef js_ProfilingStack_h
 #define js_ProfilingStack_h
 
+#include "mozilla/IntegerRange.h"
+#include "mozilla/UniquePtr.h"
+
 #include <algorithm>
 #include <stdint.h>
 
 #include "jstypes.h"
 
 #include "js/TypeDecls.h"
 #include "js/Utility.h"
 
@@ -134,16 +137,30 @@ class ProfileEntry
 
     // Bits 0...1 hold the Kind. Bits 2...3 are unused. Bits 4...12 hold the
     // Category.
     mozilla::Atomic<uint32_t, mozilla::ReleaseAcquire> kindAndCategory_;
 
     static int32_t pcToOffset(JSScript* aScript, jsbytecode* aPc);
 
   public:
+    ProfileEntry() = default;
+    ProfileEntry& operator=(const ProfileEntry& other)
+    {
+        label_ = other.label();
+        dynamicString_ = other.dynamicString();
+        void* spScript = other.spOrScript;
+        spOrScript = spScript;
+        int32_t offset = other.lineOrPcOffset;
+        lineOrPcOffset = offset;
+        uint32_t kindAndCategory = other.kindAndCategory_;
+        kindAndCategory_ = kindAndCategory;
+        return *this;
+    }
+
     enum class Kind : uint32_t {
         // A normal C++ frame.
         CPP_NORMAL = 0,
 
         // A special C++ frame indicating the start of a run of JS pseudostack
         // entries. CPP_MARKER_FOR_JS frames are ignored, except for the sp
         // field.
         CPP_MARKER_FOR_JS = 1,
@@ -305,37 +322,37 @@ class PseudoStack final
         // The label macros keep a reference to the PseudoStack to avoid a TLS
         // access. If these are somehow not all cleared we will get a
         // use-after-free so better to crash now.
         MOZ_RELEASE_ASSERT(stackPointer == 0);
     }
 
     void pushCppFrame(const char* label, const char* dynamicString, void* sp, uint32_t line,
                       js::ProfileEntry::Kind kind, js::ProfileEntry::Category category) {
-        if (stackPointer < MaxEntries) {
-            entries[stackPointer].initCppFrame(label, dynamicString, sp, line, kind, category);
-        }
+        ensureCapacityToPush([&] (js::ProfileEntry& entry) {
+            entry.initCppFrame(label, dynamicString, sp, line, kind, category);
+        });
 
         // This must happen at the end! The compiler will not reorder this
         // update because stackPointer is Atomic<..., ReleaseAcquire>, so any
         // the writes above will not be reordered below the stackPointer store.
         // Do the read and the write as two separate statements, in order to
         // make it clear that we don't need an atomic increment, which would be
         // more expensive on x86 than the separate operations done here.
         // This thread is the only one that ever changes the value of
         // stackPointer.
         uint32_t oldStackPointer = stackPointer;
         stackPointer = oldStackPointer + 1;
     }
 
     void pushJsFrame(const char* label, const char* dynamicString, JSScript* script,
                      jsbytecode* pc) {
-        if (stackPointer < MaxEntries) {
-            entries[stackPointer].initJsFrame(label, dynamicString, script, pc);
-        }
+        ensureCapacityToPush([&] (js::ProfileEntry& entry) {
+            entry.initJsFrame(label, dynamicString, script, pc);
+        });
 
         // This must happen at the end! The compiler will not reorder this
         // update because stackPointer is Atomic<..., ReleaseAcquire>, which
         // makes this assignment a release-store, so the writes above will not
         // be reordered to occur after the stackPointer store.
         // Do the read and the write as two separate statements, in order to
         // make it clear that we don't need an atomic increment, which would be
         // more expensive on x86 than the separate operations done here.
@@ -358,21 +375,55 @@ class PseudoStack final
 
     uint32_t stackSize() const { return std::min(uint32_t(stackPointer), uint32_t(MaxEntries)); }
 
   private:
     // No copying.
     PseudoStack(const PseudoStack&) = delete;
     void operator=(const PseudoStack&) = delete;
 
+    // Ensures that there's capacity to push a frame at least up to MaxEntries, and if so executes
+    // the functor with the current entry.
+    template<typename Callback>
+    void ensureCapacityToPush(const Callback& cb)
+    {
+        const size_t kInitialCapacity = 50;
+
+        MOZ_ASSERT(entryCapacity >= stackPointer);
+        if (stackPointer >= MaxEntries) {
+            return;
+        }
+
+        // Ensure that entries is always sane here.
+        if (entryCapacity == stackPointer) {
+            size_t newCapacity = entryCapacity ? entryCapacity * 2 : kInitialCapacity;
+            auto newStorage = mozilla::MakeUnique<js::ProfileEntry[]>(newCapacity);
+            for (auto i : mozilla::IntegerRange(entryCapacity)) {
+                newStorage[i] = entries[i];
+            }
+            entries = newStorage.get();
+            entryStorage = mozilla::Move(newStorage);
+            entryCapacity = newCapacity;
+        }
+        cb(entries[stackPointer]);
+    }
+
+    // The actual storage for the entries, only mutated in the current thread.
+    mozilla::UniquePtr<js::ProfileEntry[]> entryStorage;
+    size_t entryCapacity = 0;
+
   public:
     static const uint32_t MaxEntries = 1024;
 
-    // The stack entries.
-    js::ProfileEntry entries[MaxEntries];
+    // The pointer to the stack entries, this is read from the profiler thread and written from the
+    // current thread.
+    //
+    // This will almost always be a pointer to entryStorage, except during a very short period of
+    // time when we're growing it to push a frame.
+    mozilla::Atomic<js::ProfileEntry*> entries;
 
     // This may exceed MaxEntries, so instead use the stackSize() method to
     // determine the number of valid samples in entries. When this is less
     // than MaxEntries, it refers to the first free entry past the top of the
     // in-use stack (i.e. entries[stackPointer - 1] is the top stack entry).
     //
     // WARNING WARNING WARNING
     //