Bug 1574143 - Make BlocksRingBuffer::Reader RAII - r=gregtatum
authorGerald Squelart <gsquelart@mozilla.com>
Fri, 16 Aug 2019 03:55:08 +0000
changeset 488429 e54b58ba5f3a177e8d3abbb5de1e10903a83910d
parent 488428 2fd28586dfbe3ecc7a08a5dbec3cc39d2c838ee8
child 488430 90d0473fe6aa5e0ceacb3354a7e226cb7383e715
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::Reader RAII - r=gregtatum In practice the Reader doesn't need to be copied/moved/reassign. BlocksRingBuffer::Read() can just instantiate one on the stack, and pass it by reference to callbacks. Differential Revision: https://phabricator.services.mozilla.com/D42118
mozglue/baseprofiler/public/BlocksRingBuffer.h
mozglue/tests/TestBaseProfiler.cpp
--- a/mozglue/baseprofiler/public/BlocksRingBuffer.h
+++ b/mozglue/baseprofiler/public/BlocksRingBuffer.h
@@ -462,87 +462,89 @@ class BlocksRingBuffer {
     // This BlockIterator should only live inside one of the thread-safe
     // BlocksRingBuffer functions, for this reference to stay valid.
     NotNull<const BlocksRingBuffer*> mRing;
     BlockIndex mBlockIndex;
   };
 
   // Class that can create `BlockIterator`s (e.g., for range-for), or just
   // iterate through entries; lives within a lock guard lifetime.
-  class Reader {
+  class MOZ_RAII Reader {
    public:
+    Reader(const Reader&) = delete;
+    Reader& operator=(const Reader&) = delete;
+    Reader(Reader&&) = delete;
+    Reader& operator=(Reader&&) = delete;
+
 #ifdef DEBUG
     ~Reader() {
       // No Reader should live outside of a mutexed call.
-      mRing->mMutex.AssertCurrentThreadOwns();
+      mRing.mMutex.AssertCurrentThreadOwns();
     }
 #endif  // DEBUG
 
     // Index of the first block in the whole buffer.
-    BlockIndex BufferRangeStart() const { return mRing->mFirstReadIndex; }
+    BlockIndex BufferRangeStart() const { return mRing.mFirstReadIndex; }
 
     // Index past the last block in the whole buffer.
-    BlockIndex BufferRangeEnd() const { return mRing->mNextWriteIndex; }
+    BlockIndex BufferRangeEnd() const { return mRing.mNextWriteIndex; }
 
     // Iterators to the first and past-the-last blocks.
     // Compatible with range-for (see `ForEach` below as example).
     BlockIterator begin() const {
-      return BlockIterator(*mRing, BufferRangeStart());
+      return BlockIterator(mRing, BufferRangeStart());
     }
-    BlockIterator end() const {
-      return BlockIterator(*mRing, BufferRangeEnd());
-    }
+    BlockIterator end() const { return BlockIterator(mRing, BufferRangeEnd()); }
 
     // Run `aCallback(EntryReader&)` on each entry from first to last.
     // Callback should not store `EntryReader`, as it may become invalid after
     // this thread-safe call.
     template <typename Callback>
     void ForEach(Callback&& aCallback) const {
       for (EntryReader reader : *this) {
         aCallback(reader);
       }
     }
 
    private:
     friend class BlocksRingBuffer;
 
-    explicit Reader(const BlocksRingBuffer& aRing)
-        : mRing(WrapNotNull(&aRing)) {
+    explicit Reader(const BlocksRingBuffer& aRing) : mRing(aRing) {
       // No Reader should live outside of a mutexed call.
-      mRing->mMutex.AssertCurrentThreadOwns();
+      mRing.mMutex.AssertCurrentThreadOwns();
     }
 
-    // Using a non-null pointer instead of a reference, to allow copying.
     // This Reader should only live inside one of the thread-safe
     // BlocksRingBuffer functions, for this reference to stay valid.
-    NotNull<const BlocksRingBuffer*> mRing;
+    const BlocksRingBuffer& mRing;
   };
 
-  // Call `aCallback(Maybe<BlocksRingBuffer::Reader>&&)`, and return whatever
-  // `aCallback` returns. `Maybe` may be `Nothing` when out-of-session.
-  // Callback should not store `Reader`, because it may become invalid after
-  // this call.
+  // Call `aCallback(BlocksRingBuffer::Reader*)` (nullptr when out-of-session),
+  // and return whatever `aCallback` returns. Callback should not store
+  // `Reader`, because it may become invalid after this call.
   template <typename Callback>
   auto Read(Callback&& aCallback) const {
-    baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
-    Maybe<Reader> maybeReader;
-    if (MOZ_LIKELY(mMaybeUnderlyingBuffer)) {
-      maybeReader.emplace(Reader(*this));
+    {
+      baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
+      if (MOZ_LIKELY(mMaybeUnderlyingBuffer)) {
+        Reader reader(*this);
+        return std::forward<Callback>(aCallback)(&reader);
+      }
     }
-    return std::forward<Callback>(aCallback)(std::move(maybeReader));
+    return std::forward<Callback>(aCallback)(nullptr);
   }
 
   // Call `aCallback(BlocksRingBuffer::EntryReader&)` on each item.
   // Callback should not store `EntryReader`, because it may become invalid
   // after this call.
   template <typename Callback>
   void ReadEach(Callback&& aCallback) const {
-    Read([&](Maybe<Reader>&& aMaybeReader) {
-      if (MOZ_LIKELY(aMaybeReader)) {
-        std::move(aMaybeReader)->ForEach(aCallback);
+    Read([&](Reader* aReader) {
+      if (MOZ_LIKELY(aReader)) {
+        aReader->ForEach(aCallback);
       }
     });
   }
 
   // Call `aCallback(Maybe<BlocksRingBuffer::EntryReader>&&)` on the entry at
   // the given BlockIndex; The `Maybe` will be `Nothing` if out-of-session, or
   // if that entry doesn't exist anymore, or if we've reached just past the
   // last entry. Return whatever `aCallback` returns. Callback should not
@@ -878,17 +880,18 @@ class BlocksRingBuffer {
   // destructor or ClearAllEntries.
   void DestroyAllEntries() {
     mMutex.AssertCurrentThreadOwns();
     if (!mMaybeUnderlyingBuffer) {
       return;
     }
     if (mMaybeUnderlyingBuffer->mEntryDestructor) {
       // We have an entry destructor, destroy all the things!
-      Reader(*this).ForEach([this](EntryReader& aReader) {
+      Reader reader(*this);
+      reader.ForEach([this](EntryReader& aReader) {
         mMaybeUnderlyingBuffer->mEntryDestructor(aReader);
       });
     }
     mMaybeUnderlyingBuffer->mClearedBlockCount =
         mMaybeUnderlyingBuffer->mPushedBlockCount;
   }
 
   // Clear all entries, calling entry destructor (if any), and move read index
--- a/mozglue/tests/TestBaseProfiler.cpp
+++ b/mozglue/tests/TestBaseProfiler.cpp
@@ -801,18 +801,18 @@ void TestBlocksRingBufferUnderlyingBuffe
     });
     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`.
     ran = 0;
-    rb.Read([&](Maybe<BlocksRingBuffer::Reader>&& aMaybeReader) {
-      MOZ_RELEASE_ASSERT(aMaybeReader.isNothing());
+    rb.Read([&](BlocksRingBuffer::Reader* aReader) {
+      MOZ_RELEASE_ASSERT(!aReader);
       ++ran;
     });
     MOZ_RELEASE_ASSERT(ran == 1);
     ran = 0;
     rb.ReadAt(BlocksRingBuffer::BlockIndex{},
               [&](Maybe<BlocksRingBuffer::EntryReader>&& aMaybeEntryReader) {
                 MOZ_RELEASE_ASSERT(aMaybeEntryReader.isNothing());
                 ++ran;
@@ -874,18 +874,18 @@ void TestBlocksRingBufferUnderlyingBuffe
                   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{});
     ran = 0;
-    rb.Read([&](Maybe<BlocksRingBuffer::Reader>&& aMaybeReader) {
-      MOZ_RELEASE_ASSERT(aMaybeReader.isSome());
+    rb.Read([&](BlocksRingBuffer::Reader* aReader) {
+      MOZ_RELEASE_ASSERT(!!aReader);
       ++ran;
     });
     MOZ_RELEASE_ASSERT(ran == 1);
     ran = 0;
     rb.ReadEach([&](BlocksRingBuffer::EntryReader& aEntryReader) {
       MOZ_RELEASE_ASSERT(aEntryReader.RemainingBytes() == sizeof(ran));
       MOZ_RELEASE_ASSERT(aEntryReader.ReadObject<decltype(ran)>() == 1);
       ++ran;