Open Bug 1310809 Opened 8 years ago Updated 2 years ago

An annotation applied to stack S will be applied to every stack matching S

Categories

(Toolkit :: Telemetry, defect, P3)

49 Branch
defect

Tracking

()

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(2 files)

http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/xpcom/threads/BackgroundHangMonitor.cpp#429 ^-- When reporting a BHR hang X, we go through the previous stacks and look for matches to X's stack. If we find a stack that matches, we apply the annotations from X to that stack. We're attempting to fix bug 1300411 by adding annotations when we're switching tabs (see bug 1303077). Knowing how many times those stacks show up when we've annotated that we're tab switching will allow us to prioritize our efforts to optimize, but because of this bug, the frequency is polluted by hangs with the same stack that occur outside of the tab switch.
Just a heads up that billm and I are starting to look at bug 1310880 as a route for maximum accuracy on hang stacks. These patches fix real bugs in BHR, but I don't think they're as urgent as they were, say, 5 hours ago.
Comment on attachment 8801910 [details] Bug 1310809 - Compare annotations when determining whether two HangHistograms are equal. https://reviewboard.mozilla.org/r/86502/#review85640 ::: toolkit/components/telemetry/Telemetry.cpp:2780 (Diff revision 1) > + > + if (mAnnotations.length() != aOther.mAnnotations.length()) { > + return false; > + } > + > + for (size_t i = 0; i < mAnnotations.length(); ++i) { This doesn't look right: I think you need to be dereferencing the UniquePtrs before doing the comparison, or else you're comparing the pointers and not the annotation objects themselves.
Comment on attachment 8801910 [details] Bug 1310809 - Compare annotations when determining whether two HangHistograms are equal. https://reviewboard.mozilla.org/r/86502/#review85642 r=me with issues addressed
Attachment #8801910 - Flags: review?(aklotz) → review+
Comment on attachment 8801909 [details] Bug 1310809 - Store HangAnnotations in a map instead of a vector to avoid duplicates. https://reviewboard.mozilla.org/r/86500/#review85634 ::: xpcom/threads/HangAnnotations.cpp:147 (Diff revision 1) > } > > size_t > BrowserHangAnnotations::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const > { > - size_t result = sizeof(mAnnotations) + > + size_t result = sizeof(mAnnotations); This calculation should be sizeof(mAnnotations) + mAnnotations.size() * sizeof(MapType::value_type)
Comment on attachment 8801909 [details] Bug 1310809 - Store HangAnnotations in a map instead of a vector to avoid duplicates. https://reviewboard.mozilla.org/r/86500/#review85644 r=me with issues addressed.
Attachment #8801909 - Flags: review?(aklotz) → review+
Priority: -- → P3
Hey mystor, I saw that you're landing work to use the annotation stuff for BHR, and it reminded me of this old-ish bug that I guess I never landed patches for. My brain isn't fully into BHR these days - do you know if this bug is still an issue, and if these patches would still be relevant?
Flags: needinfo?(michael)
(In reply to Mike Conley (:mconley) from comment #8) > Hey mystor, I saw that you're landing work to use the annotation stuff for > BHR, and it reminded me of this old-ish bug that I guess I never landed > patches for. My brain isn't fully into BHR these days - do you know if this > bug is still an issue, and if these patches would still be relevant? Yes, it's still an issue which I'm hoping to work around. These patches might be handy to help distinguish all hangs which occur during user input events and those which don't. This problem is also a problem with native stack collection (we collect at most one native stack for all hangs with the same pseudostack), but we will probably want to work around that in a different way.
Flags: needinfo?(michael)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: