Closed Bug 805046 Opened 7 years ago Closed 7 years ago

Add a class to represent a set of stacks that share a object list

Categories

(Toolkit :: Telemetry, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: espindola, Assigned: espindola)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
Vladan and I discussed the wire format used by chrome hang. We agreed that it would be better to just make all the stacks in a report share a module list.

This patch doesn't change the wire format, but refactors the code to keep a single module list with the new CombinedProcessedStacks class.

I kept CombinedProcessedStacks distinct from HangReports because I think CombinedProcessedStacks will also be useful for reporting multiple late writes in one telemetry report.

To avoid changing the wire format at this time, the first stack that we send has all the objects and all following stacks have no objects.
Attachment #674687 - Flags: review?(vdjeric)
Comment on attachment 674687 [details] [diff] [review]
patch

>diff --git a/toolkit/components/telemetry/ProcessedStack.h b/toolkit/components/telemetry/ProcessedStack.h
>+// This class is conceptually a list of ProcessedStack objects, but it represents them
>+// more efficiently by keeping a single global list of modules.
>+class CombinedProcessedStacks {

Nit: Maybe rename to CombinedStacks?

We could merge this class with HangReports, they're both singletons and most methods from HangReports just redirect to CombinedProcessedStacks (e.g. Hangreports::AddHang, GetNumStacks, GetNumModules, GetStack, etc) plus it makes sense to keep the stacks/module map/hang durations together

>+public:
>+  typedef std::vector<Telemetry::ProcessedStack::Frame> Stack;
>+  const Telemetry::ProcessedStack::Module &GetModule(unsigned aIndex) const;
>+  size_t GetNumModules() const;
>+  const Stack &GetStack(unsigned aIndex) const;

I think we usually put the ampersand immediately after the type name

>+  void AddStack(const Telemetry::ProcessedStack &aStack);
>+  size_t GetNumStacks() const;
>+  size_t SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const;
>+private:
>+  std::vector<Telemetry::ProcessedStack::Module> mModules;
>+  std::vector<Stack> mStacks;
>+};
>+
>+size_t CombinedProcessedStacks::GetNumModules() const {
>+  return mModules.size();
>+}

Put the return type on a separate line (throughout file)

>+  size_t StackSize = aStack.GetStackSize();
>+  for (int i = 0; i < StackSize; ++i) {
>+    const Telemetry::ProcessedStack::Frame& Frame = aStack.GetFrame(i);
>+    unsigned ModIndex;

Use camel-case for variable names, e.g. "stackSize", "frame", "modIndex"
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

Nit: Use uint16_t instead of plain "unsigned"

>+class HangReports {
>+public:
>+  size_t SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const;

Nit: Rename function to GetSize() or GetReportsSize()

>+  void AddHang(const Telemetry::ProcessedStack &aStack, uint32_t aDuration);
>+  size_t GetNumStacks() const;

Nit: Rename to GetStackCount()

>+  const CombinedProcessedStacks::Stack &GetStack(unsigned aIndex) const;
>+  uint32_t GetDuration(unsigned aIndex) const;
>+  size_t GetNumModules() const;

Nit: GetModuleCount()

>+    const uint32_t moduleCount = i == 0 ? mHangReports.GetNumModules() : 0;

Nit: Put parentheses around the equality test and the rhs of assignment for a bit more readability

>     for (size_t moduleIndex = 0; moduleIndex < moduleCount; ++moduleIndex) {
>       // Current module
>       const Telemetry::ProcessedStack::Module &module =
>-        stack.GetModule(moduleIndex);
>+        mHangReports.GetModule(moduleIndex);
> 
>       JSObject *moduleInfoArray = JS_NewArrayObject(cx, 0, nullptr);
>       if (!moduleInfoArray) {
>         return NS_ERROR_FAILURE;
>       }
>       jsval val = OBJECT_TO_JSVAL(moduleInfoArray);
>       if (!JS_SetElement(cx, moduleArray, moduleIndex, &val)) {
>         return NS_ERROR_FAILURE;
>@@ -1486,31 +1601,17 @@ void
> TelemetryImpl::RecordChromeHang(uint32_t duration,
>                                 Telemetry::ProcessedStack &aStack)
> {
>   if (!sTelemetry || !sTelemetry->mCanRecord)
>     return;
> 
>   MutexAutoLock hangReportMutex(sTelemetry->mHangReportsMutex);
> 
>-  // Only report the modules which changed since the first hang report
>-  if (sTelemetry->mHangReports.Length()) {
>-    Telemetry::ProcessedStack &firstStack =
>-      sTelemetry->mHangReports[0].mStack;
>-    for (size_t i = 0; i < aStack.GetNumModules(); ++i) {
>-      const Telemetry::ProcessedStack::Module &module = aStack.GetModule(i);
>-      if (firstStack.HasModule(module)) {
>-        aStack.RemoveModule(i);
>-        --i;
>-      }
>-    }
>-  }
>-
>-  HangReport newReport = { duration, aStack };
>-  sTelemetry->mHangReports.AppendElement(newReport);
>+  sTelemetry->mHangReports.AddHang(aStack, duration);
> }
> #endif
> 
> NS_IMPL_THREADSAFE_ISUPPORTS1(TelemetryImpl, nsITelemetry)
> NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR(nsITelemetry, TelemetryImpl::CreateTelemetryInstance)
> 
> #define NS_TELEMETRY_CID \
>   {0xaea477f2, 0xb3a2, 0x469c, {0xaa, 0x29, 0x0a, 0x82, 0xd1, 0x32, 0xb8, 0x29}}
>@@ -1624,25 +1725,16 @@ size_t ProcessedStack::GetNumModules() const
> }
> 
> const ProcessedStack::Module &ProcessedStack::GetModule(unsigned aIndex) const
> {
>   MOZ_ASSERT(aIndex < mModules.size());
>   return mModules[aIndex];
> }
> 
>-bool ProcessedStack::HasModule(const Module &aModule) const {
>-  return mModules.end() !=
>-    std::find(mModules.begin(), mModules.end(), aModule);
>-}
>-
>-void ProcessedStack::RemoveModule(unsigned aIndex) {
>-  mModules.erase(mModules.begin() + aIndex);
>-}
>-
> void ProcessedStack::AddModule(const Module &aModule)
> {
>   mModules.push_back(aModule);
> }
> 
> void ProcessedStack::Clear() {
>   mModules.clear();
>   mStack.clear();
Attachment #674687 - Flags: review?(vdjeric) → review-
> Nit: Maybe rename to CombinedStacks?

Will do.

> We could merge this class with HangReports, they're both singletons and most
> methods from HangReports just redirect to CombinedProcessedStacks (e.g.
> Hangreports::AddHang, GetNumStacks, GetNumModules, GetStack, etc) plus it
> makes sense to keep the stacks/module map/hang durations together

Gah, forgot to mention when I uploaded the patch: I kept two classes because I will use CombinedStacks to report late writes, but HangReport has data that is useful only for hangs.

> >+public:
> >+  typedef std::vector<Telemetry::ProcessedStack::Frame> Stack;
> >+  const Telemetry::ProcessedStack::Module &GetModule(unsigned aIndex) const;
> >+  size_t GetNumModules() const;
> >+  const Stack &GetStack(unsigned aIndex) const;
> 
> I think we usually put the ampersand immediately after the type name

Will fix.

> >+  void AddStack(const Telemetry::ProcessedStack &aStack);
> >+  size_t GetNumStacks() const;
> >+  size_t SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const;
> >+private:
> >+  std::vector<Telemetry::ProcessedStack::Module> mModules;
> >+  std::vector<Stack> mStacks;
> >+};
> >+
> >+size_t CombinedProcessedStacks::GetNumModules() const {
> >+  return mModules.size();
> >+}
> 
> Put the return type on a separate line (throughout file)

Will do.

> >+  size_t StackSize = aStack.GetStackSize();
> >+  for (int i = 0; i < StackSize; ++i) {
> >+    const Telemetry::ProcessedStack::Frame& Frame = aStack.GetFrame(i);
> >+    unsigned ModIndex;
> 
> Use camel-case for variable names, e.g. "stackSize", "frame", "modIndex"
> https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style
> 
> Nit: Use uint16_t instead of plain "unsigned"

will do.

> >+class HangReports {
> >+public:
> >+  size_t SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const;
> 
> Nit: Rename function to GetSize() or GetReportsSize()

Can do, but all the existing functions are SizeOfExcludingThis or SizeOfIncludingThis. Are you sure I should rename this?

> >+  void AddHang(const Telemetry::ProcessedStack &aStack, uint32_t aDuration);
> >+  size_t GetNumStacks() const;
> 
> Nit: Rename to GetStackCount()

will do.

> >+  const CombinedProcessedStacks::Stack &GetStack(unsigned aIndex) const;
> >+  uint32_t GetDuration(unsigned aIndex) const;
> >+  size_t GetNumModules() const;
> 
> Nit: GetModuleCount()

will do.

> >+    const uint32_t moduleCount = i == 0 ? mHangReports.GetNumModules() : 0;
> 
> Nit: Put parentheses around the equality test and the rhs of assignment for
> a bit more readability

will do.

Will upload a new patch as soon as the build finishes.
Attached patch New patch (obsolete) — Splinter Review
Attachment #674687 - Attachment is obsolete: true
Attachment #677113 - Flags: review?(vdjeric)
Attached patch new new patch (obsolete) — Splinter Review
Noticed I forgot to change the variable names in the old one.
Attachment #677113 - Attachment is obsolete: true
Attachment #677113 - Flags: review?(vdjeric)
Attachment #677143 - Flags: review?(vdjeric)
Comment on attachment 677143 [details] [diff] [review]
new new patch

>+    Telemetry::ProcessedStack::Frame AdjustedFrame = { frame.mOffset, modIndex };
>+    adjustedStack.push_back(AdjustedFrame);

camel-case this too

>+size_t
>+CombinedStacks::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const {
>+  size_t n = 0;
>+  if (!mModules.empty())
>+    n += aMallocSizeOf(&mModules[0]);
>+  if (!mStacks.empty())
>+    n += aMallocSizeOf(&mStacks[0]);
>+  return n;
>+}

As per our discussion, this is ok if we can rely on the aMallocSizeOf function to have safeties for buffers that were not allocated with malloc or pointers that don't point to the start of a malloc'd block
Attachment #677143 - Flags: review?(vdjeric) → review+
Attached patch New patchSplinter Review
aMallocSizeOf calls malloc_usable_size on linux, and while that works on the current libstdc++ it is not safe. Would you mind doing a final review now that that is changed?

https://tbpl.mozilla.org/?tree=Try&rev=272122ca1695
Attachment #677143 - Attachment is obsolete: true
Attachment #677460 - Flags: review?(vdjeric)
Comment on attachment 677460 [details] [diff] [review]
New patch

>+size_t
>+CombinedStacks::SizeOfExcludingThis() const {
>+  // This is a crude approximation. We would like to do something like
>+  // aMallocSizeOf(&mDurations[0]), but on linux aMallocSizeOf will call
>+  // malloc_usable_size which is only safe on the pointers returned by malloc.
>+  // While it works on current libstdc++, it is better to be safe and not assume
>+  // that &vec[0] points to one. We could use a custom allocator, but
>+  // it doesn't seem worth it.

Change reference to mDurations to a member variable of this class

>+  for (std::vector<Stack>::const_iterator I = mStacks.begin(),
>+         E = mStacks.end(); I != E; ++I) {
>+    const Stack& S = *I;
>+    n += S.capacity() * sizeof(Telemetry::ProcessedStack::Frame);
>+  }

Make iterator names camel-case too
Attachment #677460 - Flags: review?(vdjeric) → review+
https://hg.mozilla.org/mozilla-central/rev/f8cd6fa12a8b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.