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

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P1)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1593112 - Updated buffer grammar to match samples storage - r?gregtatum

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1593112 - Store responsiveness between TimeBeforeCompactStack and CompactStack - r?gregtatum

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

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/13d77f977643
https://hg.mozilla.org/mozilla-central/rev/ea37282c8bdd