Closed
Bug 1261373
Opened 8 years ago
Closed 8 years ago
Add telemetry to see how long the content response takes
Categories
(Core :: Panning and Zooming, defect)
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.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45039/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45039/
Attachment #8739101 -
Flags: review?(botond)
Assignee | ||
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
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+
Assignee | ||
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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+
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/06616766e22e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 8•8 years ago
|
||
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
Assignee | ||
Comment 9•8 years ago
|
||
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 → ---
Assignee | ||
Comment 10•8 years ago
|
||
This is turning into a larger set of changes. I'll do it in a new bug.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•