Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13529009:
Add UMA metrics for measuring number of coalesed events and their latency. (Closed)
|
Created:
7 years, 8 months ago by varunjain Modified:
7 years, 8 months ago CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, derat+watch_chromium.org, flackr Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd UMA metrics for measuring number of coalesed events and their latency.
BUG=218270
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192970
Patch Set 1 #
Total comments: 18
Patch Set 2 : patch #
Total comments: 2
Patch Set 3 : patch #
Total comments: 2
Patch Set 4 : patch #
Total comments: 2
Patch Set 5 : patch #Messages
Total messages: 16 (0 generated)
Thanks Varun. https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode627 ui/base/x/x11_util.cc:627: first_coalesed_time = ui::EventTimeFromNative(last_event); I think the first coalesced event is really xev, where this event here is the 2nd. Eg. think of the case where we return 1 from this function - you could end up recording a time of about 0, which would be wrong. https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode637 ui/base/x/x11_util.cc:637: UMA_HISTOGRAM_CUSTOM_COUNTS("Event.Latency.Browser.CoalesedCount", UMA_HISTOGRAM_COUNTS_10000 should be sufficient (50 buckets instead of 100 here). https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode637 ui/base/x/x11_util.cc:637: UMA_HISTOGRAM_CUSTOM_COUNTS("Event.Latency.Browser.CoalesedCount", Event.Latency.Browser has an existing common meaning (and also Coalesced is misspelled). I'd find this simpler if we just called it "Event.CoalescedCount" and "Event.CoalescedLatency" below. https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode641 ui/base/x/x11_util.cc:641: (ui::IsMouseEvent(const_cast<XEvent*>(xev)) ? "Mouse" : "Touch")); Please add a CHECK that verifies all events here are either mouse or touch events (or add a third 'other' category). https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode641 ui/base/x/x11_util.cc:641: (ui::IsMouseEvent(const_cast<XEvent*>(xev)) ? "Mouse" : "Touch")); If we're going to separate Mouse and Touch here, then I think we should also separate it in the CoalescedCount too so we can compare them directly. https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode642 ui/base/x/x11_util.cc:642: base::TimeDelta delta = EventTimeForNow() - first_coalesed_time; The Event.Latency.Browser metrics already track the latency between event generation timestamps and consumption in the browser. Perhaps we should exclude that time here to keep the measurements separate? I.e. use last_event timestamp instead of EventTimeForNow. https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode644 ui/base/x/x11_util.cc:644: base::Histogram::FactoryGet( Isn't this fully dynamic histogram generation overkill here? Eg. couldn't you just do: UMA_HISTOGRAM_COUNTS_10000(isMouse ? "Event.CoalescedLatency.Mouse" : "Event.CoalescedLatency.Touch", delta)
https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode641 ui/base/x/x11_util.cc:641: (ui::IsMouseEvent(const_cast<XEvent*>(xev)) ? "Mouse" : "Touch")); On 2013/04/05 15:30:35, Rick Byers wrote: > If we're going to separate Mouse and Touch here, then I think we should also > separate it in the CoalescedCount too so we can compare them directly. Drive-by, wouldn't it be better / more efficient to switch between two const strings rather than construct a string?
https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode644 ui/base/x/x11_util.cc:644: base::Histogram::FactoryGet( The macros don't support this...they get confused by having different strings used. But you could have an if...else.. with two different UMA_HISTOGRAM_COUNTS() calls. On 2013/04/05 15:30:35, Rick Byers wrote: > Isn't this fully dynamic histogram generation overkill here? Eg. couldn't you > just do: > UMA_HISTOGRAM_COUNTS_10000(isMouse ? "Event.CoalescedLatency.Mouse" : > "Event.CoalescedLatency.Touch", delta)
https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode627 ui/base/x/x11_util.cc:627: first_coalesed_time = ui::EventTimeFromNative(last_event); On 2013/04/05 15:30:35, Rick Byers wrote: > I think the first coalesced event is really xev, where this event here is the > 2nd. Eg. think of the case where we return 1 from this function - you could end > up recording a time of about 0, which would be wrong. Done. https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode637 ui/base/x/x11_util.cc:637: UMA_HISTOGRAM_CUSTOM_COUNTS("Event.Latency.Browser.CoalesedCount", On 2013/04/05 15:30:35, Rick Byers wrote: > UMA_HISTOGRAM_COUNTS_10000 should be sufficient (50 buckets instead of 100 > here). Done. https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode637 ui/base/x/x11_util.cc:637: UMA_HISTOGRAM_CUSTOM_COUNTS("Event.Latency.Browser.CoalesedCount", On 2013/04/05 15:30:35, Rick Byers wrote: > Event.Latency.Browser has an existing common meaning (and also Coalesced is > misspelled). I'd find this simpler if we just called it "Event.CoalescedCount" > and "Event.CoalescedLatency" below. Done. https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode641 ui/base/x/x11_util.cc:641: (ui::IsMouseEvent(const_cast<XEvent*>(xev)) ? "Mouse" : "Touch")); On 2013/04/05 15:30:35, Rick Byers wrote: > Please add a CHECK that verifies all events here are either mouse or touch > events (or add a third 'other' category). Done. https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode641 ui/base/x/x11_util.cc:641: (ui::IsMouseEvent(const_cast<XEvent*>(xev)) ? "Mouse" : "Touch")); On 2013/04/05 15:30:35, Rick Byers wrote: > If we're going to separate Mouse and Touch here, then I think we should also > separate it in the CoalescedCount too so we can compare them directly. Done. https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode641 ui/base/x/x11_util.cc:641: (ui::IsMouseEvent(const_cast<XEvent*>(xev)) ? "Mouse" : "Touch")); On 2013/04/05 16:10:56, flackr wrote: > On 2013/04/05 15:30:35, Rick Byers wrote: > > If we're going to separate Mouse and Touch here, then I think we should also > > separate it in the CoalescedCount too so we can compare them directly. > > Drive-by, wouldn't it be better / more efficient to switch between two const > strings rather than construct a string? Done. https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode642 ui/base/x/x11_util.cc:642: base::TimeDelta delta = EventTimeForNow() - first_coalesed_time; On 2013/04/05 15:30:35, Rick Byers wrote: > The Event.Latency.Browser metrics already track the latency between event > generation timestamps and consumption in the browser. Perhaps we should exclude > that time here to keep the measurements separate? I.e. use last_event timestamp > instead of EventTimeForNow. Done. https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode644 ui/base/x/x11_util.cc:644: base::Histogram::FactoryGet( On 2013/04/05 15:30:35, Rick Byers wrote: > Isn't this fully dynamic histogram generation overkill here? Eg. couldn't you > just do: > UMA_HISTOGRAM_COUNTS_10000(isMouse ? "Event.CoalescedLatency.Mouse" : > "Event.CoalescedLatency.Touch", delta) Done. https://codereview.chromium.org/13529009/diff/1/ui/base/x/x11_util.cc#newcode644 ui/base/x/x11_util.cc:644: base::Histogram::FactoryGet( On 2013/04/05 16:46:15, DaveMoore wrote: > The macros don't support this...they get confused by having different strings > used. But you could have an if...else.. with two different > UMA_HISTOGRAM_COUNTS() calls. > On 2013/04/05 15:30:35, Rick Byers wrote: > > Isn't this fully dynamic histogram generation overkill here? Eg. couldn't you > > just do: > > UMA_HISTOGRAM_COUNTS_10000(isMouse ? "Event.CoalescedLatency.Mouse" : > > "Event.CoalescedLatency.Touch", delta) > Done.
Thanks Varun. This looks nice and simple now. LGTM with one more minor simplification suggestion. https://codereview.chromium.org/13529009/diff/9001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/13529009/diff/9001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:640: delta.InMicroseconds(), 0, 1000000, 100); For these times I think microseconds is overkill. We receive touch events at 100hz on pixel. I'd suggest just using UMA_HISTOGRAM_TIMES to simplify this even further.
+sky for OWNERS https://codereview.chromium.org/13529009/diff/9001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/13529009/diff/9001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:640: delta.InMicroseconds(), 0, 1000000, 100); On 2013/04/05 19:32:24, Rick Byers wrote: > For these times I think microseconds is overkill. We receive touch events at > 100hz on pixel. I'd suggest just using UMA_HISTOGRAM_TIMES to simplify this > even further. Done.
https://codereview.chromium.org/13529009/diff/12001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/13529009/diff/12001/ui/base/x/x11_util.cc#new... ui/base/x/x11_util.cc:647: } This function is called only for XI_Motion or XI_TouchUpdate events. So this should be either touch or mouse events. You can simply look at |event_type| and/or |tracking_id| to figure out whether it's a touch or mouse event.
https://codereview.chromium.org/13529009/diff/12001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/13529009/diff/12001/ui/base/x/x11_util.cc#new... ui/base/x/x11_util.cc:647: } On 2013/04/05 22:14:10, sadrul wrote: > This function is called only for XI_Motion or XI_TouchUpdate events. So this > should be either touch or mouse events. You can simply look at |event_type| > and/or |tracking_id| to figure out whether it's a touch or mouse event. Done.
LGTM https://codereview.chromium.org/13529009/diff/4004/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/13529009/diff/4004/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:642: DCHECK(event_type == XI_TouchUpdate); DCHECK_EQ
https://codereview.chromium.org/13529009/diff/4004/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/13529009/diff/4004/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:642: DCHECK(event_type == XI_TouchUpdate); On 2013/04/05 23:27:09, sadrul wrote: > DCHECK_EQ Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varunjain@chromium.org/13529009/17001
LGTM
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varunjain@chromium.org/13529009/17001
Message was sent while issue was closed.
Change committed as 192970 |
