Closed Bug 1261373 Opened 4 years ago Closed 4 years ago

Add telemetry to see how long the content response takes

Categories

(Core :: Panning and Zooming, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

See bug 1247280. We should add a telemetry probe to see how long it takes to get all the content response things back from the main-thread in APZ.
Comment on attachment 8739101 [details]
MozReview Request: Bug 1261373 - Record how long it takes for content response notifications to be delivered to APZ. r?

f? to bsmedberg for data collection review. This patch adds a new telemetry probe that records how long it takes for the main thread to provide the compositor thread with information that APZ needs for a new input block. We plan to use this to adjust a timeout value (currently set to 300ms) - if this value is set too low then performance will be good but correctness may suffer, and if the value is set too high then performance will be bad but correctness may be better. Getting telemetry data on what these values are in practice will allow us to tune the timeout to get maximum correctness without sacrificing too much performance.
Attachment #8739101 - Flags: feedback?(benjamin)
Comment on attachment 8739101 [details]
MozReview Request: Bug 1261373 - Record how long it takes for content response notifications to be delivered to APZ. r?

https://reviewboard.mozilla.org/r/45039/#review41637

::: gfx/layers/apz/src/InputQueue.cpp:651
(Diff revision 1)
>    for (size_t i = 0; i < mInputBlockQueue.Length(); i++) {
>      DragBlockState* block = mInputBlockQueue[i]->AsDragBlock();
>      if (block && block->GetBlockId() == aInputBlockId) {
>        block->SetDragMetrics(aDragMetrics);
>        success = block->SetConfirmedTargetApzc(aTargetApzc);
> +      block->RecordContentResponseTime();

Should DragBlockState track whether it has received this notification and override HasReceivedAllContentNotifications() accordingly?

::: toolkit/components/telemetry/Histograms.json:153
(Diff revision 1)
>    },
> +  "CONTENT_RESPONSE_DURATION" : {
> +    "alert_emails": ["kgupta@mozilla.com"],
> +    "bug_numbers": [1261373],
> +    "expires_in_version": "55",
> +    "description": "Main thread response times for APZ notifications in milliseconds",

Maybe "notification about input events" to be a little more specific?
Attachment #8739101 - Flags: review?(botond) → review+
https://reviewboard.mozilla.org/r/45039/#review41637

> Should DragBlockState track whether it has received this notification and override HasReceivedAllContentNotifications() accordingly?

In general I made HasReceivedAllContentNotifications() do the same thing as IsReadyForHandling(), minus the timeout check. DragBlockState doesn't have an overridden IsReadyForHandling(), so I didn't add a HasReceivedAllContentNotifications() either. It may be a bug that DragBlockState doesn't have this function (i.e. maybe it should return false from IsReadyForHandling() until it has a drag metrics) but that's a pre-existing bug if so, and only relevant if apz.drag.enabled is true. If we override IsReadyForHandling() then we should change this as well.

> Maybe "notification about input events" to be a little more specific?

Will do
Comment on attachment 8739101 [details]
MozReview Request: Bug 1261373 - Record how long it takes for content response notifications to be delivered to APZ. r?

I apologize for the delay.
Attachment #8739101 - Flags: feedback?(benjamin) → feedback+
https://hg.mozilla.org/mozilla-central/rev/06616766e22e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Data is starting to come in, the mobile data [1] looks kind of sane, but the desktop data [2] is not. I don't know why there's a second curve starting at 300ms, that doesn't look right. Might be a bug in my measurement code.

[1] http://mzl.la/1SH7DDg
[2] http://mzl.la/1SH7H5X
Ok, so the problem here is that a wheel input block can get multiple ContentReceivedInputBlock notifications (one for each wheel event in the block). If the block hasn't yet gotten its SetConfirmedTargetAPZC notification, all these CRIB notifications are ignored, until the 300ms timeout fires and confirms the tentative target, at which point the next CRIB notification triggers the content response duration. So yes, this is a bug in my measurement code. It should be ignoring the "confirm the tentative target" bit. I'll put a follow-up patch in this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This is turning into a larger set of changes. I'll do it in a new bug.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.