Bug 1574143 - Make BlocksRingBuffer::EntryWriter RAII - r=gregtatum
authorGerald Squelart <gsquelart@mozilla.com>
Fri, 16 Aug 2019 04:02:18 +0000
changeset 488427 1050dca058c32f29609390345646ffde7e9f6114
parent 488426 37aaf28a693732407ded4ab9e7e153bc680f02a6
child 488428 2fd28586dfbe3ecc7a08a5dbec3cc39d2c838ee8
push id113908
push userccoroiu@mozilla.com
push dateFri, 16 Aug 2019 09:57:53 +0000
treeherdermozilla-inbound@83fad6abe38a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgregtatum
bugs1574143
milestone70.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 1574143 - Make BlocksRingBuffer::EntryWriter RAII - r=gregtatum EntryWriter doesn't even need to be moveable, as BlocksRingBuffer can just create one on the stack, and pass it by reference to callbacks. This removes risks, and potential data copies. Differential Revision: https://phabricator.services.mozilla.com/D42115
mozglue/baseprofiler/public/BlocksRingBuffer.h
mozglue/tests/TestBaseProfiler.cpp
--- a/mozglue/baseprofiler/public/BlocksRingBuffer.h
+++ b/mozglue/baseprofiler/public/BlocksRingBuffer.h
@@ -561,46 +561,33 @@ class BlocksRingBuffer {
     }
     return std::forward<Callback>(aCallback)(std::move(maybeEntryReader));
   }
 
   class EntryReserver;
 
   // Class used to write an entry contents.
   // Created through `EntryReserver`, lives within a lock guard lifetime.
-  class EntryWriter : public BufferWriter {
+  class MOZ_RAII EntryWriter : public BufferWriter {
    public:
-    // Allow move-construction.
+    // Disallow copying, moving, and assignments.
+    EntryWriter(const EntryWriter& aOther) = delete;
+    EntryWriter& operator=(const EntryWriter& aOther) = delete;
+    EntryWriter(EntryWriter&& aOther) = delete;
+    EntryWriter& operator=(EntryWriter&& aOther) = delete;
+
 #ifdef DEBUG
-    EntryWriter(EntryWriter&& aOther)
-        : BufferWriter(std::move(aOther)),
-          mRing(aOther.mRing),
-          mEntryBytes(aOther.mEntryBytes),
-          mEntryStart(aOther.mEntryStart) {
-      // No EntryWriter should live outside of a mutexed call.
-      mRing.mMutex.AssertCurrentThreadOwns();
-      // In DEBUG, we need to move the moved-from EntryWriter to the end of the
-      // entry, so as not to trip the MOZ_ASSERT() in the destructor below.
-      aOther += aOther.RemainingBytes();
-    }
-
     ~EntryWriter() {
       // We expect the caller to completely fill the entry.
       // (Or at least pretend to, by moving this iterator to the end.)
       MOZ_ASSERT(RemainingBytes() == 0);
       // No EntryWriter should live outside of a mutexed call.
       mRing.mMutex.AssertCurrentThreadOwns();
     }
-#else   // DEBUG
-    EntryWriter(EntryWriter&& aOther) = default;
-#endif  // DEBUG else
-    // Disallow copying and assignments.
-    EntryWriter(const EntryWriter& aOther) = delete;
-    EntryWriter& operator=(const EntryWriter& aOther) = delete;
-    EntryWriter& operator=(EntryWriter&& aOther) = delete;
+#endif  // DEBUG
 
     // All BufferWriter (aka ModuloBuffer<uint32_t, Index>::Writer) APIs are
     // available to read/write data from/to this entry.
     // Note that there are no bound checks! So this should not be used with
     // external data.
 
     // Total number of bytes in this entry.
     Length EntryBytes() const { return mEntryBytes; }
@@ -729,17 +716,17 @@ class BlocksRingBuffer {
       mRing->mMaybeUnderlyingBuffer->mPushedBlockCount += 1;
       // Finally, let aCallback write into the entry.
       return std::forward<Callback>(aCallback)(
           EntryWriter(*mRing, blockIndex, aBytes));
     }
 
     // Write a new entry copied from the given buffer, return block index.
     BlockIndex Write(const void* aSrc, Length aBytes) {
-      return Reserve(aBytes, [&](EntryWriter aEW) {
+      return Reserve(aBytes, [&](EntryWriter& aEW) {
         aEW.Write(aSrc, aBytes);
         return aEW.CurrentBlockIndex();
       });
     }
 
     // Write a new entry copied from the given object, return block index.
     // Restricted to trivially-copyable types.
     // TODO: Allow more types (follow-up patches in progress).
@@ -804,32 +791,33 @@ class BlocksRingBuffer {
     baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
     Maybe<EntryReserver> maybeEntryReserver;
     if (MOZ_LIKELY(mMaybeUnderlyingBuffer)) {
       maybeEntryReserver.emplace(EntryReserver(*this));
     }
     return std::forward<Callback>(aCallback)(std::move(maybeEntryReserver));
   }
 
-  // Add a new entry of known size, call `aCallback` with a temporary
-  // EntryWriter, and return whatever `aCallback` returns. Callback should not
-  // store `EntryWriter`, as it may become invalid after this thread-safe call.
+  // Add a new entry of known size, call `aCallback` with a pointer to a
+  // temporary EntryWriter (can be null when out-of-session), and return
+  // whatever `aCallback` returns. Callback should not store the `EntryWriter`,
+  // as it may become invalid after this thread-safe call.
   template <typename Callback>
   auto Put(Length aLength, Callback&& aCallback) {
     return Put([&](Maybe<EntryReserver>&& aER) {
       if (MOZ_LIKELY(aER)) {
         // We are in-session, with an EntryReserver at the ready.
-        // Reserve the requested space, then invoke the callback with the given
-        // EntryWriter inserted into a Maybe.
-        return aER->Reserve(aLength, [&](EntryWriter aEW) {
-          return std::forward<Callback>(aCallback)(Some(std::move(aEW)));
+        // Reserve the requested space, then invoke the callback with a pointer
+        // to the reserved EntryWriter.
+        return aER->Reserve(aLength, [&](EntryWriter& aEW) {
+          return std::forward<Callback>(aCallback)(&aEW);
         });
       }
-      // Out-of-session, just invoke the callback with Nothing.
-      return std::forward<Callback>(aCallback)(Maybe<EntryWriter>{});
+      // Out-of-session, just invoke the callback with nullptr.
+      return std::forward<Callback>(aCallback)(nullptr);
     });
   }
 
   // Add a new entry copied from the given buffer, return block index.
   BlockIndex PutFrom(const void* aSrc, Length aBytes) {
     return Put([&](Maybe<EntryReserver>&& aER) {
       if (MOZ_LIKELY(aER)) {
         return std::move(aER)->Write(aSrc, aBytes);
--- a/mozglue/tests/TestBaseProfiler.cpp
+++ b/mozglue/tests/TestBaseProfiler.cpp
@@ -600,17 +600,17 @@ void TestBlocksRingBufferAPI() {
     MOZ_RELEASE_ASSERT(!(bi2Next < bi2));
     MOZ_RELEASE_ASSERT(!(bi2Next <= bi2));
 
     // Push `3` through EntryReserver and then EntryWriter, check writer output
     // is returned to the initial caller.
     auto put3 = rb.Put([&](Maybe<BlocksRingBuffer::EntryReserver>&& aER) {
       MOZ_RELEASE_ASSERT(aER.isSome());
       return aER->Reserve(
-          sizeof(uint32_t), [&](BlocksRingBuffer::EntryWriter aEW) {
+          sizeof(uint32_t), [&](BlocksRingBuffer::EntryWriter& aEW) {
             aEW.WriteObject(uint32_t(3));
             return float(ExtractBlockIndex(aEW.CurrentBlockIndex()));
           });
     });
     static_assert(std::is_same<decltype(put3), float>::value,
                   "Expect float as returned by callback.");
     MOZ_RELEASE_ASSERT(put3 == 11.0);
     //   0   1   2   3   4   5   6   7   8   9  10  11  12  13  14  15 (16)
@@ -668,17 +668,17 @@ void TestBlocksRingBufferAPI() {
     MOZ_RELEASE_ASSERT(count == 4);
 
     // Push 5 through EntryReserver then EntryWriter, no returns.
     // This will destroy the second entry.
     // Check that the EntryWriter can access bi4 but not bi2.
     auto bi5_6 = rb.Put([&](Maybe<BlocksRingBuffer::EntryReserver>&& aER) {
       MOZ_RELEASE_ASSERT(aER.isSome());
       return aER->Reserve(
-          sizeof(uint32_t), [&](BlocksRingBuffer::EntryWriter aEW) {
+          sizeof(uint32_t), [&](BlocksRingBuffer::EntryWriter& aEW) {
             aEW.WriteObject(uint32_t(5));
             MOZ_RELEASE_ASSERT(aEW.GetEntryAt(bi2).isNothing());
             MOZ_RELEASE_ASSERT(aEW.GetEntryAt(bi4).isSome());
             MOZ_RELEASE_ASSERT(aEW.GetEntryAt(bi4)->CurrentBlockIndex() == bi4);
             MOZ_RELEASE_ASSERT(aEW.GetEntryAt(bi4)->ReadObject<uint32_t>() ==
                                4);
             return MakePair(aEW.CurrentBlockIndex(), aEW.BlockEndIndex());
           });
@@ -799,18 +799,18 @@ void TestBlocksRingBufferUnderlyingBuffe
     // `Put()` functions run the callback with `Nothing`.
     int32_t ran = 0;
     rb.Put([&](Maybe<BlocksRingBuffer::EntryReserver>&& aMaybeEntryReserver) {
       MOZ_RELEASE_ASSERT(aMaybeEntryReserver.isNothing());
       ++ran;
     });
     MOZ_RELEASE_ASSERT(ran == 1);
     ran = 0;
-    rb.Put(1, [&](Maybe<BlocksRingBuffer::EntryWriter>&& aMaybeEntryWriter) {
-      MOZ_RELEASE_ASSERT(aMaybeEntryWriter.isNothing());
+    rb.Put(1, [&](BlocksRingBuffer::EntryWriter* aMaybeEntryWriter) {
+      MOZ_RELEASE_ASSERT(!aMaybeEntryWriter);
       ++ran;
     });
     MOZ_RELEASE_ASSERT(ran == 1);
     // `PutFrom` won't do anything, and returns the null BlockIndex.
     MOZ_RELEASE_ASSERT(rb.PutFrom(&ran, sizeof(ran)) ==
                        BlocksRingBuffer::BlockIndex{});
     MOZ_RELEASE_ASSERT(rb.PutObject(ran) == BlocksRingBuffer::BlockIndex{});
     // `Read()` functions run the callback with `Nothing`.
@@ -878,18 +878,18 @@ void TestBlocksRingBufferUnderlyingBuffe
     rb.Put([&](Maybe<BlocksRingBuffer::EntryReserver>&& aMaybeEntryReserver) {
       MOZ_RELEASE_ASSERT(aMaybeEntryReserver.isSome());
       ++ran;
     });
     MOZ_RELEASE_ASSERT(ran == 1);
     ran = 0;
     // The following three `Put...` will write three int32_t of value 1.
     bi = rb.Put(sizeof(ran),
-                [&](Maybe<BlocksRingBuffer::EntryWriter>&& aMaybeEntryWriter) {
-                  MOZ_RELEASE_ASSERT(aMaybeEntryWriter.isSome());
+                [&](BlocksRingBuffer::EntryWriter* aMaybeEntryWriter) {
+                  MOZ_RELEASE_ASSERT(!!aMaybeEntryWriter);
                   ++ran;
                   aMaybeEntryWriter->WriteObject(ran);
                   return aMaybeEntryWriter->CurrentBlockIndex();
                 });
     MOZ_RELEASE_ASSERT(ran == 1);
     MOZ_RELEASE_ASSERT(rb.PutFrom(&ran, sizeof(ran)) !=
                        BlocksRingBuffer::BlockIndex{});
     MOZ_RELEASE_ASSERT(rb.PutObject(ran) != BlocksRingBuffer::BlockIndex{});
@@ -1036,18 +1036,18 @@ void TestBlocksRingBufferThreading() {
     threads[threadNo] = std::thread(
         [&](int aThreadNo) {
           ::SleepMilli(1);
           constexpr int pushCount = 1024;
           for (int push = 0; push < pushCount; ++push) {
             // Reserve as many bytes as the thread number (but at least enough
             // to store an int), and write an increasing int.
             rb.Put(std::max(aThreadNo, int(sizeof(push))),
-                   [&](Maybe<BlocksRingBuffer::EntryWriter>&& aEW) {
-                     MOZ_RELEASE_ASSERT(aEW.isSome());
+                   [&](BlocksRingBuffer::EntryWriter* aEW) {
+                     MOZ_RELEASE_ASSERT(!!aEW);
                      aEW->WriteObject(aThreadNo * 1000000 + push);
                      *aEW += aEW->RemainingBytes();
                    });
           }
         },
         threadNo);
   }