Closed Bug 1593112 Opened 5 years ago Closed 5 years ago

Responsiveness is wrongly copied into duplicated samples (when thread is sleeping)

Categories

(Core :: Gecko Profiler, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 --- wontfix
firefox72 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Run MOZ_PROFILER_STARTUP=1 MOZ_PROFILER_SHUTDOWN=profile.json ./mach run and quit soon after launch.
Samples show that the responsiveness seems to be copied from one sample to the next when the stack is duplicated, when it shouldn't be.
E.g.:

"samples": {
"schema": {"stack": 0, "time": 1, "responsiveness": 2},
"data": [
...
[494, 797.1160000000001, 44.181000000000154],
[523, 799.1741999999999, 46.23919999999998],
[523, 801.1120000000001, 46.23919999999998],

Notice that in the last sample, the stack is the same, the time was incremented correctly, but the responsiveness is present and exactly the same as in the previous sample, which is wrong -- it should either be absent, or increment by the same amount as the time.

Thank you :jesup for noticing.
I'm suspecting it may be due to my recent change of profiler buffer storage; I'll investigate shortly.

The issue started with bug 1581049, where we started storing full periodic samples, including responsiveness, inside a single CompactStack entry.
So when a thread is sleeping we blindly duplicate that stack with the same responsiveness, instead of removing or increasing it.

The easiest solution is to store the responsiveness separately so it doesn't get duplicated.

Keywords: regression
Regressed by: 1581049

The responsiveness was stored with periodic stack samples.
Since bug 1581049, periodic stack samples are now contained within a single
"modern" CompactStack entry.
When a thread is sleeping, CompactStacks are simply copied verbatim, including
the responsiveness, which doesn't make sense because since we're sleeping the
responsiveness should increase.

To avoid duplicating the responsiveness, it is now stored as a separate buffer
entry before the CompactStack.

Depends on D51553

Randell, I've got a patch ready to fix this issue: https://bugzilla.mozilla.org/attachment.cgi?id=9106090
But I see you may be ready to finally land bug 1572337, which could make this one here obsolete or in need of rebasing.
Please let me know if I should go ahead with mine now anyway, or wait for yours first.

Flags: needinfo?(rjesup)

I commented on the patch. The biggest issue is naming I think (which is no big deal - there's no 'perfect' name here); either one of us can reform our patch to work on the other's. I'm still waiting on review of comment changes; don't let me stop you from landing.

Flags: needinfo?(rjesup)
Attachment #9106090 - Attachment description: Bug 1593112 - Store responsiveness between TimeBeforeCompactStack and CompactStack - → Bug 1593112 - Store responsiveness between TimeBeforeCompactStack and CompactStack - r?gregtatum
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13d77f977643
Updated buffer grammar to match samples storage - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/ea37282c8bdd
Store responsiveness between TimeBeforeCompactStack and CompactStack - r=gregtatum
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: