Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
681763003:
Add input latency histograms for wheel events (Closed)
|
Created:
6 years, 1 month ago by ccameron Modified:
6 years, 1 month ago Reviewers:
jdduke (slow), sadrul, sky, Alexei Svitkine (slow), Yufeng Shen (Slow to review), jar (doing other things) CC:
chromium-reviews, ben+aura_chromium.org, tdresser+watch_chromium.org, jam, darin-cc_chromium.org, kalyank, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd input latency histograms for wheel events
This works for touch events, but not for wheel events. Use the existing
touch latency machinery to enable logging of wheel events.
Update ComputeInputLatencyHistograms to report latency of
INPUT_EVENT_LATENCY_ACK_RWH_COMPONENT in the absence of a
INPUT_EVENT_LATENCY_UI_COMPONENT, as it is still valid.
BUG=224781
Committed: https://crrev.com/19687827d2cbe7bdc377ed3785ce5331b179d091
Cr-Commit-Position: refs/heads/master@{#302518}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Incorporate review feedback #Patch Set 3 : More cleanup #
Total comments: 6
Patch Set 4 : fix HISTOGRAM macro issue #
Total comments: 2
Patch Set 5 : Do counters the right way #
Total comments: 2
Patch Set 6 : Correct name #
Total comments: 2
Patch Set 7 : Fix ., indent, and XML #
Total comments: 2
Patch Set 8 : Add comments about histograms #
Total comments: 8
Patch Set 9 : DCHECK changes #
Total comments: 3
Patch Set 10 : Use switch instead #
Total comments: 1
Patch Set 11 : Fix space #
Messages
Total messages: 49 (11 generated)
ccameron@chromium.org changed reviewers: + jdduke@chromium.org, miletus@chromium.org, sadrul@chromium.org
Adding jdduke/miletus for latency Adding sadrul for event OWNER.
https://codereview.chromium.org/681763003/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/681763003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.cc:1950: CreateInputAckLatencyInfoAndHistogram("Touch", &touch_event.latency); Please add the acked component before adding the ui::INPUT_EVENT_LATENCY_TERMINATED_TOUCH_COMPONENT above. When adding ui::INPUT_EVENT_LATENCY_TERMINATED_TOUCH_COMPONENT, because it is a terminal component so all the LatencyInfo components will be dumped into trace. Those components added after won't make to the trace. Same for the wheel ack event/TERMINATED_MOUSE_COMPONENT.
Thanks -- updated
https://codereview.chromium.org/681763003/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/681763003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:485: ui::LatencyInfo CreateInputEventLatencyInfoIfNotExist( why do you need to change the name ?
https://codereview.chromium.org/681763003/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/681763003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:485: ui::LatencyInfo CreateInputEventLatencyInfoIfNotExist( On 2014/10/28 18:02:25, Yufeng Shen wrote: > why do you need to change the name ? It relates to input, and that wasn't in the name. The fact that it's RWH-centric is clear from the fact that this is RenderWidgetHostImpl. Let me know if you'd prefer another name.
latency_info lgtm
Adding sky@ for ui/ stamp.
https://codereview.chromium.org/681763003/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/681763003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:490: void ComputeInputLatencyHistograms( Could be wrong, but I don't think this needs to be a member function. Can we make it a free function in the .cc file anon namespace? Can you also use a const char* arg type for the name?
https://codereview.chromium.org/681763003/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/681763003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:490: void ComputeInputLatencyHistograms( On 2014/10/29 01:47:37, jdduke wrote: > Could be wrong, but I don't think this needs to be a member function. Can we > make it a free function in the .cc file anon namespace? Can you also use a const > char* arg type for the name? It calls to GetLatencyComponentId, which is a member function. Changed to const char*.
https://codereview.chromium.org/681763003/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/681763003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:490: void ComputeInputLatencyHistograms( On 2014/10/29 02:09:02, ccameron1 wrote: > On 2014/10/29 01:47:37, jdduke wrote: > > Could be wrong, but I don't think this needs to be a member function. Can we > > make it a free function in the .cc file anon namespace? Can you also use a > const > > char* arg type for the name? > > It calls to GetLatencyComponentId, which is a member function. > > Changed to const char*. Could you pass GetLatencyComponentId() to the function? RWHI has enough going on that isolating these latency methods (and potential side effects) will almost always be a win =/.
https://codereview.chromium.org/681763003/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/681763003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.h:490: void ComputeInputLatencyHistograms( On 2014/10/29 02:18:37, jdduke wrote: > On 2014/10/29 02:09:02, ccameron1 wrote: > > On 2014/10/29 01:47:37, jdduke wrote: > > > Could be wrong, but I don't think this needs to be a member function. Can we > > > make it a free function in the .cc file anon namespace? Can you also use a > > const > > > char* arg type for the name? > > > > It calls to GetLatencyComponentId, which is a member function. > > > > Changed to const char*. > > Could you pass GetLatencyComponentId() to the function? RWHI has enough going on > that isolating these latency methods (and potential side effects) will almost > always be a win =/. I've marked this function const, to indicate that it does not mutate the object.
sky@chromium.org changed reviewers: + sky@chromium.org
LGTM
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/681763003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Turns out the HISTOGRAM macros must take a static argument (each instance of the macro must always get the same name passed to it -- you can't put any variables there). This makes things a bit messier (big switch statements). Let me know if you see any cleaner ways.
https://codereview.chromium.org/681763003/diff/100001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/681763003/diff/100001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2107: switch (type) { Does this help ? https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
Updated. https://codereview.chromium.org/681763003/diff/100001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/681763003/diff/100001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2107: switch (type) { On 2014/10/30 20:42:25, Yufeng Shen wrote: > Does this help ? > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > Thanks, yeah, that's the way it's supposed to be done.
https://codereview.chromium.org/681763003/diff/120001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/681763003/diff/120001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2105: 1, there should be no "." between Touch/Wheel and UI
Thanks! https://codereview.chromium.org/681763003/diff/120001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/681763003/diff/120001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2105: 1, On 2014/10/30 20:55:46, Yufeng Shen wrote: > there should be no "." between Touch/Wheel and UI Done.
https://codereview.chromium.org/681763003/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/681763003/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2120: base::StringPrintf("Event.Latency.Browser.%s.Acked", type), and the "." here ? Also forgot to mention that you also need to modify the src/tools/metrics/histograms/histograms.xml to annotate the histogram you want to collect, see here https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...
Patchset #9 (id:160001) has been deleted
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #7 (id:180001) has been deleted
ccameron@chromium.org changed reviewers: + jar@chromium.org
Thanks (eep) -- updated. Adding jar for the histograms.xml review https://codereview.chromium.org/681763003/diff/140001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/681763003/diff/140001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2120: base::StringPrintf("Event.Latency.Browser.%s.Acked", type), On 2014/10/30 21:18:40, Yufeng Shen wrote: > and the "." here ? > > Also forgot to mention that you also need to modify the > src/tools/metrics/histograms/histograms.xml > to annotate the histogram you want to collect, see here > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist... Done.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/681763003/diff/200001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/681763003/diff/200001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:491: const char* event_type_name, Can you make this param an enum? Then, you can switch on it in the body of the function to get the right string. This way, if someone wants to add another type of entry here, they'll need to update the enum/string switch, which should have a comment to also update histograms.xml. Speaking of which, please add the histograms for Touch to the XML file too.
https://codereview.chromium.org/681763003/diff/200001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/681763003/diff/200001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:491: const char* event_type_name, On 2014/10/31 19:00:39, Alexei Svitkine wrote: > Can you make this param an enum? > > Then, you can switch on it in the body of the function to get the right string. > This way, if someone wants to add another type of entry here, they'll need to > update the enum/string switch, which should have a comment to also update > histograms.xml. PS4 had this approach, but that was revised to the current approach. I've added a comment above ComputeInputLatencyHistogram to indicate that new entries in the XML file are needed as new event types are used. > Speaking of which, please add the histograms for Touch to the XML file too. The Touch entries are already present (just above the Wheel event entries).
lgtm with a suggestion https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:492: // XML file any time histograms for a new event type are added. Can you document in the comment the current strings this is called with? That way, it's clear when a new one is added that it needs to be updated.
Sorry for the drive-by... https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2099: DCHECK(ui_component.event_count == 1); Nit: DCHECK_EQ, also above with rwh_component (while you're at it =/). https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2104: base::StringPrintf("Event.Latency.Browser.%sUI", type), It's a bit unfortunate to be doing two sprintf's for every single event here (not to mention the histogram fetch), so I'm sympathetic to using an enum switch or having a |const char* GetUIHistogramName(event_type)| and |const char* GetAckedHistogramName(event_type)|, where event_type is the WebInputEvent::Type and you can DCHECK for invalid types.
https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2099: DCHECK(ui_component.event_count == 1); On 2014/10/31 20:46:40, jdduke wrote: > Nit: DCHECK_EQ, also above with rwh_component (while you're at it =/). Done. https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2104: base::StringPrintf("Event.Latency.Browser.%sUI", type), On 2014/10/31 20:46:41, jdduke wrote: > It's a bit unfortunate to be doing two sprintf's for every single event here > (not to mention the histogram fetch), so I'm sympathetic to using an enum switch > or having a |const char* GetUIHistogramName(event_type)| and |const char* > GetAckedHistogramName(event_type)|, where event_type is the WebInputEvent::Type > and you can DCHECK for invalid types. This matches what we're doing for all events, already, at https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... It should be noted that this is being done in the context of an input event and 2 IPCs (the input and the input ACK), so I have a hard time believing that manually unrolling these histograms as switch statements would be the right side of the CPU cycles vs code-readability/correctness/maintainability tradeoff to go on. I've added a DCHECK to ensure that the type name is "Wheel" or "Touch", along with a comment next to the DCHECK saying ("If you want something else, add it to the XML file"). https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:492: // XML file any time histograms for a new event type are added. On 2014/10/31 20:18:20, Alexei Svitkine wrote: > Can you document in the comment the current strings this is called with? That > way, it's clear when a new one is added that it needs to be updated. I put a DCHECK that this be "Touch" or "Wheel" next to a comment saying "if you want more, add them to the XML file".
https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2104: base::StringPrintf("Event.Latency.Browser.%sUI", type), On 2014/10/31 22:13:05, ccameron1 wrote: > On 2014/10/31 20:46:41, jdduke wrote: > > It's a bit unfortunate to be doing two sprintf's for every single event here > > (not to mention the histogram fetch), so I'm sympathetic to using an enum > switch > > or having a |const char* GetUIHistogramName(event_type)| and |const char* > > GetAckedHistogramName(event_type)|, where event_type is the > WebInputEvent::Type > > and you can DCHECK for invalid types. > > This matches what we're doing for all events, already, at > We probably shouldn't be doing it there either. > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > It should be noted that this is being done in the context of an input event and > 2 IPCs (the input and the input ACK), so I have a hard time believing that > manually unrolling these histograms as switch statements would be the right side > of the CPU cycles vs code-readability/correctness/maintainability tradeoff to go > on. > This line of reasoning does not help the situation on mobile devices, where death by a thousand cuts is a very real thing. It's not clear to me how a helper routine for computing the name hurts readability here, but I'm not an owner of this file and won't complain further...
Thanks all! https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/681763003/diff/220001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2104: base::StringPrintf("Event.Latency.Browser.%sUI", type), On 2014/10/31 22:23:32, jdduke wrote: > On 2014/10/31 22:13:05, ccameron1 wrote: > > On 2014/10/31 20:46:41, jdduke wrote: > > > It's a bit unfortunate to be doing two sprintf's for every single event here > > > (not to mention the histogram fetch), so I'm sympathetic to using an enum > > switch > > > or having a |const char* GetUIHistogramName(event_type)| and |const char* > > > GetAckedHistogramName(event_type)|, where event_type is the > > WebInputEvent::Type > > > and you can DCHECK for invalid types. > > > > This matches what we're doing for all events, already, at > > > > We probably shouldn't be doing it there either. > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > > > It should be noted that this is being done in the context of an input event > and > > 2 IPCs (the input and the input ACK), so I have a hard time believing that > > manually unrolling these histograms as switch statements would be the right > side > > of the CPU cycles vs code-readability/correctness/maintainability tradeoff to > go > > on. > > > > This line of reasoning does not help the situation on mobile devices, where > death by a thousand cuts is a very real thing. It's not clear to me how a helper > routine for computing the name hurts readability here, but I'm not an owner of > this file and won't complain further... > Well, I had things that way in patchset 4: https://codereview.chromium.org/681763003/diff/100001/content/browser/rendere... It seemed a bit too verbose (I even ended up adding the "." typos in that patchset & not noticing it), so the other way was suggested. This is an area where having an owner/dictator clearly say "you will do it this way" the first time around would help.
FWIW, I don't think the patch set 4 solution is necessarily the only way to do it with enums. For example, we could keep the enum but avoid sprintf by still using the non-macro histogram API and switching just to assign the histogram name strings. On Oct 31, 2014 7:46 PM, <ccameron@chromium.org> wrote: > Thanks all! > > > https://codereview.chromium.org/681763003/diff/220001/ > content/browser/renderer_host/render_widget_host_impl.cc > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > https://codereview.chromium.org/681763003/diff/220001/ > content/browser/renderer_host/render_widget_host_impl.cc#newcode2104 > content/browser/renderer_host/render_widget_host_impl.cc:2104: > base::StringPrintf("Event.Latency.Browser.%sUI", type), > On 2014/10/31 22:23:32, jdduke wrote: > >> On 2014/10/31 22:13:05, ccameron1 wrote: >> > On 2014/10/31 20:46:41, jdduke wrote: >> > > It's a bit unfortunate to be doing two sprintf's for every single >> > event here > >> > > (not to mention the histogram fetch), so I'm sympathetic to using >> > an enum > >> > switch >> > > or having a |const char* GetUIHistogramName(event_type)| and >> > |const char* > >> > > GetAckedHistogramName(event_type)|, where event_type is the >> > WebInputEvent::Type >> > > and you can DCHECK for invalid types. >> > >> > This matches what we're doing for all events, already, at >> > >> > > We probably shouldn't be doing it there either. >> > > > >> > > https://code.google.com/p/chromium/codesearch#chromium/ > src/content/renderer/render_widget.cc&l=1005 > >> > >> > It should be noted that this is being done in the context of an >> > input event > >> and >> > 2 IPCs (the input and the input ACK), so I have a hard time >> > believing that > >> > manually unrolling these histograms as switch statements would be >> > the right > >> side >> > of the CPU cycles vs code-readability/correctness/maintainability >> > tradeoff to > >> go >> > on. >> > >> > > This line of reasoning does not help the situation on mobile devices, >> > where > >> death by a thousand cuts is a very real thing. It's not clear to me >> > how a helper > >> routine for computing the name hurts readability here, but I'm not an >> > owner of > >> this file and won't complain further... >> > > > Well, I had things that way in patchset 4: > > https://codereview.chromium.org/681763003/diff/100001/ > content/browser/renderer_host/render_widget_host_impl.cc > > It seemed a bit too verbose (I even ended up adding the "." typos in > that patchset & not noticing it), so the other way was suggested. > > This is an area where having an owner/dictator clearly say "you will do > it this way" the first time around would help. > > https://codereview.chromium.org/681763003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/681763003/diff/240001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/681763003/diff/240001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2086: const char* type, If this is latency sensitive.... which I'm guessing it is... you'd be better off passing in an enum, and then switch()ing to select one of N UMA_HISTOGRAM macro invocations. As it stands now, the lookups you repeatedly induce in this function (to find yet-again an existing histogram) will use locks, make a map search/access, verify string matches, etc. The UMA macro eliminates all of that, and provides much reduced run-time cost. https://codereview.chromium.org/681763003/diff/240001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2102: 0, nit: undent 1 space https://codereview.chromium.org/681763003/diff/240001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2109: base::StringPrintf("Event.Latency.Browser.%sUI", type), nit: I'd also expect the repeated use of StringPrintf to be a large run-time cost waste. For the limited input values to this method, there are much simpler/faster ways to do this.
Okay, in that case, it seems like we should go back to the scheme from patchest 4 -- I've moved back to that one.
lgtm, thanks
https://codereview.chromium.org/681763003/diff/260001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/681763003/diff/260001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2098: 0, nit: indentation.
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/681763003/280001
Message was sent while issue was closed.
Committed patchset #11 (id:280001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/19687827d2cbe7bdc377ed3785ce5331b179d091 Cr-Commit-Position: refs/heads/master@{#302518} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
