Responsiveness is wrongly copied into duplicated samples (when thread is sleeping)
Categories
(Core :: Gecko Profiler, defect, P1)
Tracking
()
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.
Assignee | ||
Comment 1•5 years ago
•
|
||
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.
Assignee | ||
Comment 2•5 years ago
|
||
Assignee | ||
Comment 3•5 years ago
|
||
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, CompactStack
s 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
Assignee | ||
Comment 4•5 years ago
|
||
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.
Comment 5•5 years ago
|
||
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.
Updated•5 years ago
|
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
Comment 7•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/13d77f977643
https://hg.mozilla.org/mozilla-central/rev/ea37282c8bdd
Updated•5 years ago
|
Updated•2 years ago
|
Description
•