Closed Bug 1357742 Opened 3 years ago Closed 3 years ago
Deal with accumulations of INPUT
_EVENT _RESPONSE _MS across sleep/wake
Input events may come in before a computer goes to sleep and not be handled until after it wakes. On platforms where TimeStamp measures elapsed time (like on Windows), this can result in incorrectly-long values being accumulated into INPUT_EVENT_RESPONSE_MS. So let's not accumulate those. They're wrong. How to handle this: 1) Just ditch all input events created before sleep and handled after wake 2) Try to be clever and subtract the sleep duration from the handling time I'm tempted to go for #1 since users are less likely to care about the response time to an input event that happens across a sleep/wake cycle. Also, it's easier and I expect not many events will get caught by this.
For reference, sleep notifies NS_WIDGET_SLEEP_OBSERVER_TOPIC and wake notifies NS_WIDGET_WAKE_OBSERVER_TOPIC : https://dxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#226
Comment on attachment 8859620 [details] bug 1357742 - Drop INPUT_EVENT_RESPONSE_MS measurements across sleep/wake https://reviewboard.mozilla.org/r/131630/#review134408 Seems like rather edge case. Does this actually cause some issues in telemetry?
Attachment #8859620 - Flags: review?(bugs) → review+
We have no way to know if it's causing issues until we shut down this avenue of error :)
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/1e9b46dbf4b5 Drop INPUT_EVENT_RESPONSE_MS measurements across sleep/wake r=smaug
Well, are we getting weird looking data?
We're seeing a low mean time before failure (when by "failure" we mean "input lag of over 10s") which is making us wonder, at the very least. Other concerns about these sorts of measures are broached on bug 1357457. The idea is to polish these to a high sheen of certainty so they can be used as indicators of product quality.
You should probably backport this to beta since we are using this telemetry to evaluate chrome input event latency.
Would it be worth keeping an eye on it for a week or so in Nightly first to see if the distribution actually changes?
This change should probably either rename the histogram or at least be noted in the documentation. Will this patch also affect the COALESCED version in the other bug? With a _V2 histogram rename, I'd suggest uplifting immediately or along with the COALESCED bug. Without, I'd suggest doing some nightly comparisons first.
I think we should have definitely renamed the histogram before landing this, but I'm late to the game... Now that this landed, not sure, up to you. But with or without renaming, we should validate the data before using it -- renaming just makes it unnecessary to wait before uplifting.
Comment on attachment 8859620 [details] bug 1357742 - Drop INPUT_EVENT_RESPONSE_MS measurements across sleep/wake Approval Request Comment [Feature/Bug causing the regression]: N/A [User impact if declined]: A key measurement for quantum release criteria will not be baselineable. [Is this code covered by automated tests?]: Telemetry alerts and hindsight will notify us if something goes awry with data collection [Has the fix been verified in Nightly?]: I have verified that data is still coming through as it used to. [Needs manual test from QE? If yes, steps to reproduce]: Would be pretty tricky to manually test. [List of other uplifts needed for the feature/fix]: bug 1357457 [Is the change risky?]: Nope [Why is the change risky/not risky?]: It's small, self-contained, and has little effect on the data collected (while simultaneously greatly improving our confidence in the numbers it reports) [String changes made/needed]: None
Comment on attachment 8859620 [details] bug 1357742 - Drop INPUT_EVENT_RESPONSE_MS measurements across sleep/wake A key measurement for quantum release criteria. Beta54+. Should be in 54 beta 5.
(In reply to Chris H-C :chutten from comment #14) > [Is this code covered by automated tests?]: Telemetry alerts and hindsight > will notify us if something goes awry with data collection > [Has the fix been verified in Nightly?]: I have verified that data is still > coming through as it used to. > [Needs manual test from QE? If yes, steps to reproduce]: Would be pretty > tricky to manually test. Setting qe-verify- based on Chris' assessment on manual testing needs and the fact that this fix has telemetry coverage.
Out of curiosity, why dont we return NS_OK in this case?
...likely due to a mistake on my part. I've filed bug 1390263 to rectify it. Thanks for the catch!
You need to log in before you can comment on or make changes to this bug.