From 90b45a2617f41b19cc6aa7e3125b5c61c3ac9527 Mon Sep 17 00:00:00 2001 From: Gerald Squelart Date: Mon, 22 Aug 2022 21:31:48 +0000 Subject: [PATCH] Bug 1785745 - Make UniqueStack::mUniqueStrings and mCodeAddressService private - r=canaltinova This makes it easier to understand how these are actually used, and constrain these uses as intended. Differential Revision: https://phabricator.services.mozilla.com/D154960 --- .../baseprofiler/core/ProfileBufferEntry.h | 7 ++- .../baseprofiler/core/ProfiledThreadData.cpp | 5 +- tools/profiler/core/ProfileBufferEntry.cpp | 48 +++++++++++-------- tools/profiler/core/ProfileBufferEntry.h | 11 ++++- tools/profiler/core/ProfiledThreadData.cpp | 9 ++-- 5 files changed, 47 insertions(+), 33 deletions(-) diff --git a/mozglue/baseprofiler/core/ProfileBufferEntry.h b/mozglue/baseprofiler/core/ProfileBufferEntry.h index 73ff8f86006e..48c7ba772a1d 100644 --- a/mozglue/baseprofiler/core/ProfileBufferEntry.h +++ b/mozglue/baseprofiler/core/ProfileBufferEntry.h @@ -204,14 +204,17 @@ class UniqueStacks { void SpliceFrameTableElements(SpliceableJSONWriter& aWriter); void SpliceStackTableElements(SpliceableJSONWriter& aWriter); + UniqueJSONStrings& UniqueStrings() { + MOZ_RELEASE_ASSERT(mUniqueStrings.get()); + return *mUniqueStrings; + } + private: void StreamNonJITFrame(const FrameKey& aFrame); void StreamStack(const StackKey& aStack); - public: UniquePtr mUniqueStrings; - private: SpliceableChunkedJSONWriter mFrameTableWriter; HashMap mFrameToIndexMap; diff --git a/mozglue/baseprofiler/core/ProfiledThreadData.cpp b/mozglue/baseprofiler/core/ProfiledThreadData.cpp index 0617cee64ee4..e57888a34be6 100644 --- a/mozglue/baseprofiler/core/ProfiledThreadData.cpp +++ b/mozglue/baseprofiler/core/ProfiledThreadData.cpp @@ -31,8 +31,7 @@ void ProfiledThreadData::StreamJSON(const ProfileBuffer& aBuffer, double aSinceTime) { UniqueStacks uniqueStacks; - MOZ_ASSERT(uniqueStacks.mUniqueStrings); - aWriter.SetUniqueStrings(*uniqueStacks.mUniqueStrings); + aWriter.SetUniqueStrings(uniqueStacks.UniqueStrings()); aWriter.Start(); { @@ -78,7 +77,7 @@ void ProfiledThreadData::StreamJSON(const ProfileBuffer& aBuffer, aWriter.StartArrayProperty("stringTable"); { - std::move(*uniqueStacks.mUniqueStrings) + std::move(uniqueStacks.UniqueStrings()) .SpliceStringTableElements(aWriter); } aWriter.EndArray(); diff --git a/tools/profiler/core/ProfileBufferEntry.cpp b/tools/profiler/core/ProfileBufferEntry.cpp index 051dd5cf0c15..7cba45dd8357 100644 --- a/tools/profiler/core/ProfileBufferEntry.cpp +++ b/tools/profiler/core/ProfileBufferEntry.cpp @@ -390,6 +390,29 @@ void UniqueStacks::SpliceStackTableElements(SpliceableJSONWriter& aWriter) { aWriter.TakeAndSplice(mStackTableWriter.TakeChunkedWriteFunc()); } +[[nodiscard]] nsAutoCString UniqueStacks::FunctionNameOrAddress(void* aPC) { + nsAutoCString nameOrAddress; + + if (!mCodeAddressService || + !mCodeAddressService->GetFunction(aPC, nameOrAddress) || + nameOrAddress.IsEmpty()) { + nameOrAddress.AppendASCII("0x"); + // `AppendInt` only knows `uint32_t` or `uint64_t`, but because these are + // just aliases for *two* of (`unsigned`, `unsigned long`, and `unsigned + // long long`), a call with `uintptr_t` could use the third type and + // therefore would be ambiguous. + // So we want to force using exactly `uint32_t` or `uint64_t`, whichever + // matches the size of `uintptr_t`. + // (The outer cast to `uint` should then be a no-op.) + using uint = std::conditional_t; + nameOrAddress.AppendInt(static_cast(reinterpret_cast(aPC)), + 16); + } + + return nameOrAddress; +} + void UniqueStacks::StreamStack(const StackKey& aStack) { enum Schema : uint32_t { PREFIX = 0, FRAME = 1 }; @@ -999,28 +1022,11 @@ ProfilerThreadId ProfileBuffer::DoStreamSamplesAndMarkersToJSON( void* pc = e.Get().GetPtr(); e.Next(); - nsAutoCString buf; + nsAutoCString functionNameOrAddress = + uniqueStacks.FunctionNameOrAddress(pc); - if (!uniqueStacks.mCodeAddressService || - !uniqueStacks.mCodeAddressService->GetFunction(pc, buf) || - buf.IsEmpty()) { - buf.AppendASCII("0x"); - // `AppendInt` only knows `uint32_t` or `uint64_t`, but because - // these are just aliases for *two* of (`unsigned`, `unsigned - // long`, and `unsigned long long`), a call with `uintptr_t` could - // use the third type and therefore would be ambiguous. - // So we want to force using exactly `uint32_t` or `uint64_t`, - // whichever matches the size of `uintptr_t`. - // (The outer cast to `uint` should then be a no-op.) - using uint = - std::conditional_t; - buf.AppendInt(static_cast(reinterpret_cast(pc)), - 16); - } - - stack = uniqueStacks.AppendFrame(stack, - UniqueStacks::FrameKey(buf.get())); + stack = uniqueStacks.AppendFrame( + stack, UniqueStacks::FrameKey(functionNameOrAddress.get())); } else if (e.Get().IsLabel()) { numFrames++; diff --git a/tools/profiler/core/ProfileBufferEntry.h b/tools/profiler/core/ProfileBufferEntry.h index c12426f118dd..5a8b7ed44d58 100644 --- a/tools/profiler/core/ProfileBufferEntry.h +++ b/tools/profiler/core/ProfileBufferEntry.h @@ -347,16 +347,23 @@ class UniqueStacks { void SpliceFrameTableElements(SpliceableJSONWriter& aWriter); void SpliceStackTableElements(SpliceableJSONWriter& aWriter); + [[nodiscard]] UniqueJSONStrings& UniqueStrings() { + MOZ_RELEASE_ASSERT(mUniqueStrings.get()); + return *mUniqueStrings; + } + + // Find the function name at the given PC (if a ProfilerCodeAddressService was + // provided), otherwise just stringify that PC. + [[nodiscard]] nsAutoCString FunctionNameOrAddress(void* aPC); + private: void StreamNonJITFrame(const FrameKey& aFrame); void StreamStack(const StackKey& aStack); - public: mozilla::UniquePtr mUniqueStrings; ProfilerCodeAddressService* mCodeAddressService = nullptr; - private: SpliceableChunkedJSONWriter mFrameTableWriter; mozilla::HashMap mFrameToIndexMap; diff --git a/tools/profiler/core/ProfiledThreadData.cpp b/tools/profiler/core/ProfiledThreadData.cpp index 596c65419a86..85e62e8031a0 100644 --- a/tools/profiler/core/ProfiledThreadData.cpp +++ b/tools/profiler/core/ProfiledThreadData.cpp @@ -86,7 +86,7 @@ static void StreamTables(UniqueStacks&& aUniqueStacks, JSContext* aCx, aWriter.StartArrayProperty("stringTable"); { aProgressLogger.SetLocalProgress(60_pc, "Splicing string table..."); - std::move(*aUniqueStacks.mUniqueStrings).SpliceStringTableElements(aWriter); + std::move(aUniqueStacks.UniqueStrings()).SpliceStringTableElements(aWriter); aProgressLogger.SetLocalProgress(90_pc, "Spliced string table"); } aWriter.EndArray(); @@ -140,8 +140,7 @@ void ProfiledThreadData::StreamJSON( 0_pc, "Preparing unique stacks...", 10_pc, "Prepared Unique stacks")); - MOZ_ASSERT(uniqueStacks->mUniqueStrings); - aWriter.SetUniqueStrings(*uniqueStacks->mUniqueStrings); + aWriter.SetUniqueStrings(uniqueStacks->UniqueStrings()); aWriter.Start(); { @@ -390,9 +389,9 @@ ThreadStreamingContext::ThreadStreamingContext( aProgressLogger.CreateSubLoggerFromTo( 0_pc, "Preparing thread streaming context unique stacks...", 99_pc, "Prepared thread streaming context Unique stacks"))) { - mSamplesDataWriter.SetUniqueStrings(*mUniqueStacks->mUniqueStrings); + mSamplesDataWriter.SetUniqueStrings(mUniqueStacks->UniqueStrings()); mSamplesDataWriter.StartBareList(); - mMarkersDataWriter.SetUniqueStrings(*mUniqueStacks->mUniqueStrings); + mMarkersDataWriter.SetUniqueStrings(mUniqueStacks->UniqueStrings()); mMarkersDataWriter.StartBareList(); }