Skip to content

Commit

Permalink
Bug 1580091 - Use BaseProfilerMaybeMutex in BlocksRingBuffer - r=greg…
Browse files Browse the repository at this point in the history 
…tatum

In some situations we will *need* to use a `BlocksRingBuffer` that absolutely
does not use a mutex -- In particular in the critical section of
`SamplerThread::Run()`, when a thread is suspended, because any action on any
mutex (even a private one that no-one else interacts with) can result in a hang.

As a bonus, `BlocksRingBuffer`s that are known not to be used in multi-thread
situations (e.g., backtraces, extracted stacks for duplication, etc.) will waste
less resources trying to lock/unlock their mutex.

Differential Revision: https://phabricator.services.mozilla.com/D45305

--HG--
extra : moz-landing-system : lando
  • Loading branch information
@squelart
squelart committed Sep 18, 2019
1 parent cac9b16 commit c90cc7f
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 42 deletions.
88 changes: 53 additions & 35 deletions mozglue/baseprofiler/public/BlocksRingBuffer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,25 +169,34 @@ class BlocksRingBuffer {
Index mBlockIndex;
};

enum class ThreadSafety { WithoutMutex, WithMutex };

// Default constructor starts out-of-session (nothing to read or write).
BlocksRingBuffer() = default;
explicit BlocksRingBuffer(ThreadSafety aThreadSafety)
: mMutex(aThreadSafety != ThreadSafety::WithoutMutex) {}

// Constructors with no entry destructor, the oldest entries will be silently
// overwritten/destroyed.

// Create a buffer of the given length.
explicit BlocksRingBuffer(PowerOfTwo<Length> aLength)
: mMaybeUnderlyingBuffer(Some(UnderlyingBuffer(aLength))) {}
explicit BlocksRingBuffer(ThreadSafety aThreadSafety,
PowerOfTwo<Length> aLength)
: mMutex(aThreadSafety != ThreadSafety::WithoutMutex),
mMaybeUnderlyingBuffer(Some(UnderlyingBuffer(aLength))) {}

// Take ownership of an existing buffer.
BlocksRingBuffer(UniquePtr<Buffer::Byte[]> aExistingBuffer,
BlocksRingBuffer(ThreadSafety aThreadSafety,
UniquePtr<Buffer::Byte[]> aExistingBuffer,
PowerOfTwo<Length> aLength)
: mMaybeUnderlyingBuffer(
: mMutex(aThreadSafety != ThreadSafety::WithoutMutex),
mMaybeUnderlyingBuffer(
Some(UnderlyingBuffer(std::move(aExistingBuffer), aLength))) {}

// Use an externally-owned buffer.
BlocksRingBuffer(Buffer::Byte* aExternalBuffer, PowerOfTwo<Length> aLength)
: mMaybeUnderlyingBuffer(
BlocksRingBuffer(ThreadSafety aThreadSafety, Buffer::Byte* aExternalBuffer,
PowerOfTwo<Length> aLength)
: mMutex(aThreadSafety != ThreadSafety::WithoutMutex),
mMaybeUnderlyingBuffer(
Some(UnderlyingBuffer(aExternalBuffer, aLength))) {}

// Constructors with an entry destructor, which will be called with an
Expand All @@ -198,26 +207,32 @@ class BlocksRingBuffer {

// Create a buffer of the given length.
template <typename EntryDestructor>
explicit BlocksRingBuffer(PowerOfTwo<Length> aLength,
explicit BlocksRingBuffer(ThreadSafety aThreadSafety,
PowerOfTwo<Length> aLength,
EntryDestructor&& aEntryDestructor)
: mMaybeUnderlyingBuffer(Some(UnderlyingBuffer(
: mMutex(aThreadSafety != ThreadSafety::WithoutMutex),
mMaybeUnderlyingBuffer(Some(UnderlyingBuffer(
aLength, std::forward<EntryDestructor>(aEntryDestructor)))) {}

// Take ownership of an existing buffer.
template <typename EntryDestructor>
explicit BlocksRingBuffer(UniquePtr<Buffer::Byte[]> aExistingBuffer,
explicit BlocksRingBuffer(ThreadSafety aThreadSafety,
UniquePtr<Buffer::Byte[]> aExistingBuffer,
PowerOfTwo<Length> aLength,
EntryDestructor&& aEntryDestructor)
: mMaybeUnderlyingBuffer(Some(UnderlyingBuffer(
: mMutex(aThreadSafety != ThreadSafety::WithoutMutex),
mMaybeUnderlyingBuffer(Some(UnderlyingBuffer(
std::move(aExistingBuffer), aLength,
std::forward<EntryDestructor>(aEntryDestructor)))) {}

// Use an externally-owned buffer.
template <typename EntryDestructor>
explicit BlocksRingBuffer(Buffer::Byte* aExternalBuffer,
explicit BlocksRingBuffer(ThreadSafety aThreadSafety,
Buffer::Byte* aExternalBuffer,
PowerOfTwo<Length> aLength,
EntryDestructor&& aEntryDestructor)
: mMaybeUnderlyingBuffer(Some(UnderlyingBuffer(
: mMutex(aThreadSafety != ThreadSafety::WithoutMutex),
mMaybeUnderlyingBuffer(Some(UnderlyingBuffer(
aExternalBuffer, aLength,
std::forward<EntryDestructor>(aEntryDestructor)))) {}

Expand All @@ -226,43 +241,43 @@ class BlocksRingBuffer {
~BlocksRingBuffer() {
#ifdef DEBUG
// Needed because of lock DEBUG-check in `DestroyAllEntries()`.
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
#endif // DEBUG
DestroyAllEntries();
}

// Remove underlying buffer, if any.
void Reset() {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
ResetUnderlyingBuffer();
}

// Create a buffer of the given length.
void Set(PowerOfTwo<Length> aLength) {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
ResetUnderlyingBuffer();
mMaybeUnderlyingBuffer.emplace(aLength);
}

// Take ownership of an existing buffer.
void Set(UniquePtr<Buffer::Byte[]> aExistingBuffer,
PowerOfTwo<Length> aLength) {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
ResetUnderlyingBuffer();
mMaybeUnderlyingBuffer.emplace(std::move(aExistingBuffer), aLength);
}

// Use an externally-owned buffer.
void Set(Buffer::Byte* aExternalBuffer, PowerOfTwo<Length> aLength) {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
ResetUnderlyingBuffer();
mMaybeUnderlyingBuffer.emplace(aExternalBuffer, aLength);
}

// Create a buffer of the given length, with entry destructor.
template <typename EntryDestructor>
void Set(PowerOfTwo<Length> aLength, EntryDestructor&& aEntryDestructor) {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
ResetUnderlyingBuffer();
mMaybeUnderlyingBuffer.emplace(
aLength, std::forward<EntryDestructor>(aEntryDestructor));
Expand All @@ -272,7 +287,7 @@ class BlocksRingBuffer {
template <typename EntryDestructor>
void Set(UniquePtr<Buffer::Byte[]> aExistingBuffer,
PowerOfTwo<Length> aLength, EntryDestructor&& aEntryDestructor) {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
ResetUnderlyingBuffer();
mMaybeUnderlyingBuffer.emplace(
std::move(aExistingBuffer), aLength,
Expand All @@ -283,25 +298,27 @@ class BlocksRingBuffer {
template <typename EntryDestructor>
void Set(Buffer::Byte* aExternalBuffer, PowerOfTwo<Length> aLength,
EntryDestructor&& aEntryDestructor) {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
ResetUnderlyingBuffer();
mMaybeUnderlyingBuffer.emplace(
aExternalBuffer, aLength,
std::forward<EntryDestructor>(aEntryDestructor));
}

bool IsThreadSafe() const { return mMutex.IsActivated(); }

// Lock the buffer mutex and run the provided callback.
// This can be useful when the caller needs to explicitly lock down this
// buffer, but not do anything else with it.
template <typename Callback>
auto LockAndRun(Callback&& aCallback) const {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
return std::forward<Callback>(aCallback)();
}

// Buffer length in bytes.
Maybe<PowerOfTwo<Length>> BufferLength() const {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
return mMaybeUnderlyingBuffer.map([](const UnderlyingBuffer& aBuffer) {
return aBuffer.mBuffer.BufferLength();
});
Expand Down Expand Up @@ -344,7 +361,7 @@ class BlocksRingBuffer {
// Note that these may change right after this thread-safe call, so they
// should only be used for statistical purposes.
State GetState() const {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
return {
mFirstReadIndex, mNextWriteIndex,
mMaybeUnderlyingBuffer ? mMaybeUnderlyingBuffer->mPushedBlockCount : 0,
Expand Down Expand Up @@ -642,7 +659,7 @@ class BlocksRingBuffer {
template <typename Callback>
auto Read(Callback&& aCallback) const {
{
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
if (MOZ_LIKELY(mMaybeUnderlyingBuffer)) {
Reader reader(*this);
return std::forward<Callback>(aCallback)(&reader);
Expand Down Expand Up @@ -670,7 +687,7 @@ class BlocksRingBuffer {
// store `EntryReader`, because it may become invalid after this call.
template <typename Callback>
auto ReadAt(BlockIndex aBlockIndex, Callback&& aCallback) const {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
MOZ_ASSERT(aBlockIndex <= mNextWriteIndex);
Maybe<EntryReader> maybeEntryReader;
if (MOZ_LIKELY(mMaybeUnderlyingBuffer) && aBlockIndex >= mFirstReadIndex &&
Expand Down Expand Up @@ -814,7 +831,7 @@ class BlocksRingBuffer {
template <typename CallbackBytes, typename Callback>
auto ReserveAndPut(CallbackBytes aCallbackBytes, Callback&& aCallback) {
{ // Locked block.
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
if (MOZ_LIKELY(mMaybeUnderlyingBuffer)) {
Length bytes = std::forward<CallbackBytes>(aCallbackBytes)();
// Don't allow even half of the buffer length. More than that would
Expand Down Expand Up @@ -904,15 +921,15 @@ class BlocksRingBuffer {
// Clear all entries, calling entry destructor (if any), and move read index
// to the end so that these entries cannot be read anymore.
void Clear() {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
ClearAllEntries();
}

// Clear all entries strictly before aBlockIndex, calling calling entry
// destructor (if any), and move read index to the end so that these entries
// cannot be read anymore.
void ClearBefore(BlockIndex aBlockIndex) {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
if (!mMaybeUnderlyingBuffer) {
return;
}
Expand Down Expand Up @@ -957,7 +974,7 @@ class BlocksRingBuffer {

#ifdef DEBUG
void Dump() const {
baseprofiler::detail::BaseProfilerAutoLock lock(mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(mMutex);
if (!mMaybeUnderlyingBuffer) {
printf("empty BlocksRingBuffer\n");
return;
Expand Down Expand Up @@ -1087,7 +1104,7 @@ class BlocksRingBuffer {
friend struct Deserializer<UniquePtr<BlocksRingBuffer>>;

// Mutex guarding the following members.
mutable baseprofiler::detail::BaseProfilerMutex mMutex;
mutable baseprofiler::detail::BaseProfilerMaybeMutex mMutex;

struct UnderlyingBuffer {
// Create a buffer of the given length.
Expand Down Expand Up @@ -1869,7 +1886,7 @@ struct BlocksRingBuffer::Deserializer<Variant<Ts...>> {
template <>
struct BlocksRingBuffer::Serializer<BlocksRingBuffer> {
static Length Bytes(const BlocksRingBuffer& aBuffer) {
baseprofiler::detail::BaseProfilerAutoLock lock(aBuffer.mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(aBuffer.mMutex);
if (aBuffer.mMaybeUnderlyingBuffer.isNothing()) {
// Out-of-session, we only need 1 byte to store a length of 0.
return ULEB128Size<Length>(0);
Expand All @@ -1887,7 +1904,7 @@ struct BlocksRingBuffer::Serializer<BlocksRingBuffer> {
}

static void Write(EntryWriter& aEW, const BlocksRingBuffer& aBuffer) {
baseprofiler::detail::BaseProfilerAutoLock lock(aBuffer.mMutex);
baseprofiler::detail::BaseProfilerMaybeAutoLock lock(aBuffer.mMutex);
if (aBuffer.mMaybeUnderlyingBuffer.isNothing()) {
// Out-of-session, only store a length of 0.
aEW.WriteULEB128<Length>(0);
Expand Down Expand Up @@ -2014,8 +2031,9 @@ struct BlocksRingBuffer::Deserializer<UniquePtr<BlocksRingBuffer>> {
return bufferUPtr;
}
// We have a non-empty buffer.
// allocate an empty BlocksRingBuffer.
bufferUPtr = MakeUnique<BlocksRingBuffer>();
// allocate an empty BlocksRingBuffer without mutex.
bufferUPtr = MakeUnique<BlocksRingBuffer>(
BlocksRingBuffer::ThreadSafety::WithoutMutex);
// Rewind the reader before the length and deserialize the contents, using
// the non-UniquePtr Deserializer.
aER -= ULEB128Size(len);
Expand Down
17 changes: 11 additions & 6 deletions mozglue/tests/TestBaseProfiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,8 @@ void TestBlocksRingBufferAPI() {

// Start a temporary block to constrain buffer lifetime.
{
BlocksRingBuffer rb(&buffer[MBSize], MakePowerOfTwo32<MBSize>(),
BlocksRingBuffer rb(BlocksRingBuffer::ThreadSafety::WithMutex,
&buffer[MBSize], MakePowerOfTwo32<MBSize>(),
[&](BlocksRingBuffer::EntryReader& aReader) {
lastDestroyed = aReader.ReadObject<uint32_t>();
});
Expand Down Expand Up @@ -854,7 +855,7 @@ void TestBlocksRingBufferUnderlyingBufferChanges() {
printf("TestBlocksRingBufferUnderlyingBufferChanges...\n");

// Out-of-session BlocksRingBuffer to start with.
BlocksRingBuffer rb;
BlocksRingBuffer rb(BlocksRingBuffer::ThreadSafety::WithMutex);

// Block index to read at. Initially "null", but may be changed below.
BlocksRingBuffer::BlockIndex bi;
Expand Down Expand Up @@ -1060,7 +1061,8 @@ void TestBlocksRingBufferThreading() {
for (size_t i = 0; i < MBSize * 3; ++i) {
buffer[i] = uint8_t('A' + i);
}
BlocksRingBuffer rb(&buffer[MBSize], MakePowerOfTwo32<MBSize>(),
BlocksRingBuffer rb(BlocksRingBuffer::ThreadSafety::WithMutex,
&buffer[MBSize], MakePowerOfTwo32<MBSize>(),
[&](BlocksRingBuffer::EntryReader& aReader) {
lastDestroyed = aReader.ReadObject<int>();
});
Expand Down Expand Up @@ -1148,7 +1150,8 @@ void TestBlocksRingBufferSerialization() {
for (size_t i = 0; i < MBSize * 3; ++i) {
buffer[i] = uint8_t('A' + i);
}
BlocksRingBuffer rb(&buffer[MBSize], MakePowerOfTwo32<MBSize>());
BlocksRingBuffer rb(BlocksRingBuffer::ThreadSafety::WithMutex,
&buffer[MBSize], MakePowerOfTwo32<MBSize>());

// Will expect literal string to always have the same address.
# define THE_ANSWER "The answer is "
Expand Down Expand Up @@ -1278,15 +1281,17 @@ void TestBlocksRingBufferSerialization() {
for (size_t i = 0; i < MBSize2 * 3; ++i) {
buffer2[i] = uint8_t('B' + i);
}
BlocksRingBuffer rb2(&buffer2[MBSize2], MakePowerOfTwo32<MBSize2>());
BlocksRingBuffer rb2(BlocksRingBuffer::ThreadSafety::WithoutMutex,
&buffer2[MBSize2], MakePowerOfTwo32<MBSize2>());
rb2.PutObject(rb);

// 3rd BlocksRingBuffer deserialized from the 2nd one.
uint8_t buffer3[MBSize * 3];
for (size_t i = 0; i < MBSize * 3; ++i) {
buffer3[i] = uint8_t('C' + i);
}
BlocksRingBuffer rb3(&buffer3[MBSize], MakePowerOfTwo32<MBSize>());
BlocksRingBuffer rb3(BlocksRingBuffer::ThreadSafety::WithoutMutex,
&buffer3[MBSize], MakePowerOfTwo32<MBSize>());
rb2.ReadEach(
[&](BlocksRingBuffer::EntryReader& aER) { aER.ReadIntoObject(rb3); });

Expand Down
3 changes: 2 additions & 1 deletion tools/profiler/tests/gtest/GeckoProfiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ TEST(BaseProfiler, BlocksRingBuffer)
for (size_t i = 0; i < MBSize * 3; ++i) {
buffer[i] = uint8_t('A' + i);
}
BlocksRingBuffer rb(&buffer[MBSize], MakePowerOfTwo32<MBSize>());
BlocksRingBuffer rb(BlocksRingBuffer::ThreadSafety::WithMutex,
&buffer[MBSize], MakePowerOfTwo32<MBSize>());

{
nsCString cs(NS_LITERAL_CSTRING("nsCString"));
Expand Down

0 comments on commit c90cc7f

Please sign in to comment.