Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16213002:
Add telemetry to track the time an event spent waiting for the main thread. (Closed)
|
Created:
7 years, 6 months ago by varunjain Modified:
7 years, 1 month ago Reviewers:
jamesr, jdduke (slow), nduca, sadrul, Ken Russell (switch to Gerrit), brianderson, Rick Byers, jbauman CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd telemetry to track the time an event spent waiting for the main thread.
BUG=244251
Patch Set 1 #Patch Set 2 : patch #Patch Set 3 : patch #
Total comments: 2
Patch Set 4 : patch #
Total comments: 6
Patch Set 5 : patch #
Total comments: 4
Patch Set 6 : patch #Patch Set 7 : patch #
Total comments: 4
Patch Set 8 : patch #
Total comments: 2
Patch Set 9 : patch #
Total comments: 4
Patch Set 10 : rebased #
Total comments: 6
Patch Set 11 : patch #Patch Set 12 : patch #
Total comments: 4
Patch Set 13 : patch #Patch Set 14 : patch #
Total comments: 4
Patch Set 15 : patch #Patch Set 16 : minor fixes #
Total comments: 5
Patch Set 17 : patch #
Total comments: 4
Messages
Total messages: 57 (0 generated)
On 2013/05/30 00:01:04, varunjain wrote: Actually I just realized that thus cl is wrong :( This will always diff against the dispatch time of the last event. The renderer-dispatch time should really be a queue of times... Or I will see if I can use latencyinfo. In any case, please don't review for now.
https://codereview.chromium.org/16213002/diff/9001/content/renderer/gpu/input... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/9001/content/renderer/gpu/input... content/renderer/gpu/input_event_filter.cc:58: this, messages_.front())); You can send base::TimeTicks::Now() with base::Bind. So you won't need renderer_thread_dispatch_time_, and multiple events arriving wouldn't affect this. https://codereview.chromium.org/16213002/diff/9001/content/renderer/gpu/input... content/renderer/gpu/input_event_filter.cc:121: int64 delta = now - renderer_thread_dispatch_time_; just: base::TimeDelta delta = TimeTicks::Now() - <param>; int64 delta_micros = delta.InMicroseconds(); ...
On 2013/05/30 00:50:56, sadrul wrote: > https://codereview.chromium.org/16213002/diff/9001/content/renderer/gpu/input... > File content/renderer/gpu/input_event_filter.cc (right): > > https://codereview.chromium.org/16213002/diff/9001/content/renderer/gpu/input... > content/renderer/gpu/input_event_filter.cc:58: this, messages_.front())); > You can send base::TimeTicks::Now() with base::Bind. So you won't need > renderer_thread_dispatch_time_, and multiple events arriving wouldn't affect > this. > > https://codereview.chromium.org/16213002/diff/9001/content/renderer/gpu/input... > content/renderer/gpu/input_event_filter.cc:121: int64 delta = now - > renderer_thread_dispatch_time_; > just: > base::TimeDelta delta = TimeTicks::Now() - <param>; > int64 delta_micros = delta.InMicroseconds(); > > ... Ha! thanks sadrul! CL ready for review again!
LGTM https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:128: WebInputEvent::getEventName(event->type)), You are going to need to change this too :) https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:131: 100, nit: add const ints for 1000000 and 100 and use in both places.
lgtm
Please ask nduca and brianderson for reviews before committing. How expensive is recording of an UMA stat? Is it going to add even more latency? LGTM from my standpoint.
On 2013/05/30 11:07:30, kbr wrote: > Please ask nduca and brianderson for reviews before committing. How expensive is > recording of an UMA stat? Is it going to add even more latency? > > LGTM from my standpoint. UMA is designed to be very light weight. We do per-event UMA stat at a few other places as well.
UMA metrics are lightweight, so I am not concerned about adding them. I am concerned about duplication of work here though. John Bauman has been implementing a LatencyInfo struct and passing it along the various paths input events can take. I think it would make more sense to add UMA reporting to LatencyInfo. If there is a path we want to measure that isn't already covered by LatencyInfo, we should plumb LatencyInfo there rather than adding UMA metrics directly.
On 2013/05/30 18:28:00, Brian Anderson wrote: > UMA metrics are lightweight, so I am not concerned about adding them. I am > concerned about duplication of work here though. > > John Bauman has been implementing a LatencyInfo struct and passing it along the > various paths input events can take. I think it would make more sense to add UMA > reporting to LatencyInfo. If there is a path we want to measure that isn't > already covered by LatencyInfo, we should plumb LatencyInfo there rather than > adding UMA metrics directly. I think LatencyInfo is awesome but I dont see any need for it here. The LatencyInfo I will be adding for these timestamps will not be used anywhere downstream. That said, if we are switching to that model regardless of the case, I'll be happy to switch here.
On 2013/05/30 18:33:25, varunjain wrote: > On 2013/05/30 18:28:00, Brian Anderson wrote: > > UMA metrics are lightweight, so I am not concerned about adding them. I am > > concerned about duplication of work here though. > > > > John Bauman has been implementing a LatencyInfo struct and passing it along > the > > various paths input events can take. I think it would make more sense to add > UMA > > reporting to LatencyInfo. If there is a path we want to measure that isn't > > already covered by LatencyInfo, we should plumb LatencyInfo there rather than > > adding UMA metrics directly. > > I think LatencyInfo is awesome but I dont see any need for it here. The > LatencyInfo I will be adding for these timestamps will not be used anywhere > downstream. That said, if we are switching to that model regardless of the case, > I'll be happy to switch here. In general it makes sense to use LatencyInfo, but in this particular case I think it would be fine to do it this way. It would be more work to put the data into the LatencyInfo in the IPC message than it would be to just pass it along, and it's going to be consumed immediately anyway.
We have some already existing UMA metrics tracking event latency (they've been shipping for a few months). It may be worth it to merge these efforts. On 2013/05/30 20:50:49, jbauman wrote: > On 2013/05/30 18:33:25, varunjain wrote: > > On 2013/05/30 18:28:00, Brian Anderson wrote: > > > UMA metrics are lightweight, so I am not concerned about adding them. I am > > > concerned about duplication of work here though. > > > > > > John Bauman has been implementing a LatencyInfo struct and passing it along > > the > > > various paths input events can take. I think it would make more sense to add > > UMA > > > reporting to LatencyInfo. If there is a path we want to measure that isn't > > > already covered by LatencyInfo, we should plumb LatencyInfo there rather > than > > > adding UMA metrics directly. > > > > I think LatencyInfo is awesome but I dont see any need for it here. The > > LatencyInfo I will be adding for these timestamps will not be used anywhere > > downstream. That said, if we are switching to that model regardless of the > case, > > I'll be happy to switch here. > > In general it makes sense to use LatencyInfo, but in this particular case I > think it would be fine to do it this way. It would be more work to put the data > into the LatencyInfo in the IPC message than it would be to just pass it along, > and it's going to be consumed immediately anyway.
On 2013/05/30 20:50:49, jbauman wrote: > On 2013/05/30 18:33:25, varunjain wrote: > > On 2013/05/30 18:28:00, Brian Anderson wrote: > > > UMA metrics are lightweight, so I am not concerned about adding them. I am > > > concerned about duplication of work here though. > > > > > > John Bauman has been implementing a LatencyInfo struct and passing it along > > the > > > various paths input events can take. I think it would make more sense to add > > UMA > > > reporting to LatencyInfo. If there is a path we want to measure that isn't > > > already covered by LatencyInfo, we should plumb LatencyInfo there rather > than > > > adding UMA metrics directly. > > > > I think LatencyInfo is awesome but I dont see any need for it here. The > > LatencyInfo I will be adding for these timestamps will not be used anywhere > > downstream. That said, if we are switching to that model regardless of the > case, > > I'll be happy to switch here. > > In general it makes sense to use LatencyInfo, but in this particular case I > think it would be fine to do it this way. It would be more work to put the data > into the LatencyInfo in the IPC message than it would be to just pass it along, > and it's going to be consumed immediately anyway. lgtm then!
I really would have preferred to see some refactoring of this code to make collecting this kind of metric more easy. This doesn't give us, for instance, a trace event for the same thing, or cleanup latency tracing efforts at all. You're just adding something you need --- adding complexity --- without much net win. Just my $0.02.
On 2013/05/30 21:37:01, nduca wrote: > I really would have preferred to see some refactoring of this code to make > collecting this kind of metric more easy. This doesn't give us, for instance, a > trace event for the same thing, or cleanup latency tracing efforts at all. > You're just adding something you need --- adding complexity --- without much net > win. Just my $0.02. I agree we should aim to have our key latency metrics be available to tracing, telemetry and UMA using some common infrastructure. The granularity of events should probably be different between these tools, but I think UMA data should be a strict subset of what we can get through telemetry and tracing (as soon as we see a regression in UMA numbers, we're going to need tracing and/or telemetry to track it down anyway). Let's discuss further over e-mail (and VC if necessary). I'll start a thread now. Rick
https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:127: base::StringPrintf("Event.Latency.ImplThreadToRendererThread.%s", doing a runtime StringPrintf() every time we hit this codepath is really inefficient when we can only generate one of ~30 strings fixed strings. please fix https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.h (right): https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.h:74: const base::TimeTicks& dispatch_time); pass TimeTicks by value, not const ref. it's just an int64
On 2013/05/31 19:36:31, jamesr wrote: > https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... > File content/renderer/gpu/input_event_filter.cc (right): > > https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... > content/renderer/gpu/input_event_filter.cc:127: > base::StringPrintf("Event.Latency.ImplThreadToRendererThread.%s", > doing a runtime StringPrintf() every time we hit this codepath is really > inefficient when we can only generate one of ~30 strings fixed strings. please > fix I don't think there's any reason to think the distribution of this histogram will change depending on the type of event. I suggest we just keep this simple and use a single event. > https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... > File content/renderer/gpu/input_event_filter.h (right): > > https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... > content/renderer/gpu/input_event_filter.h:74: const base::TimeTicks& > dispatch_time); > pass TimeTicks by value, not const ref. it's just an int64
On 2013/06/01 01:19:21, Rick Byers wrote: > On 2013/05/31 19:36:31, jamesr wrote: > > > https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... > > File content/renderer/gpu/input_event_filter.cc (right): > > > > > https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... > > content/renderer/gpu/input_event_filter.cc:127: > > base::StringPrintf("Event.Latency.ImplThreadToRendererThread.%s", > > doing a runtime StringPrintf() every time we hit this codepath is really > > inefficient when we can only generate one of ~30 strings fixed strings. please > > fix > > I don't think there's any reason to think the distribution of this histogram > will change depending on the type of event. I suggest we just keep this simple > and use a single event. Sorry, I mean a single histogram combined for all event types. > > > https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... > > File content/renderer/gpu/input_event_filter.h (right): > > > > > https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... > > content/renderer/gpu/input_event_filter.h:74: const base::TimeTicks& > > dispatch_time); > > pass TimeTicks by value, not const ref. it's just an int64
On 2013/05/30 21:37:01, nduca wrote: > I really would have preferred to see some refactoring of this code to make > collecting this kind of metric more easy. This doesn't give us, for instance, a > trace event for the same thing, or cleanup latency tracing efforts at all. > You're just adding something you need --- adding complexity --- without much net > win. Just my $0.02. For what it's worth, the net win is to help us understand why we have a pretty long tail in the Event.Latency.Renderer.GestureScrollUpdate UMA stat. From ad hoc testing we _think_ the things that could result it a GSU taking so long to get there are 1) time spent running JavaScript event handlers, and 2) time where the main (or compositor) threads are busy, eg. painting blocking the handling of events But it's possible we've got some other perf problem we don't understand. This stat should help us understand if there something else going on specific to the input event pipeline that we need to dig into.
https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:127: base::StringPrintf("Event.Latency.ImplThreadToRendererThread.%s", On 2013/05/31 19:36:32, jamesr wrote: > doing a runtime StringPrintf() every time we hit this codepath is really > inefficient when we can only generate one of ~30 strings fixed strings. please > fix Removed per-event metric. No strings needed anymore. https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.h (right): https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.h:74: const base::TimeTicks& dispatch_time); On 2013/05/31 19:36:32, jamesr wrote: > pass TimeTicks by value, not const ref. it's just an int64 Done.
not quite https://codereview.chromium.org/16213002/diff/33002/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/33002/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:120: int64 delta = base::TimeTicks::Now().ToInternalValue() - dispatch_time; you shouldn't do math on internal representations if you can at all help it. in this case, you can - use a base::TimeDelta to represent the difference between two times and then pass that to the histogram macro - it understands base::TimeDelta https://codereview.chromium.org/16213002/diff/33002/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.h (right): https://codereview.chromium.org/16213002/diff/33002/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.h:73: void ForwardToMainListener(const IPC::Message& message, int64 dispatch_time); base::TimeTicks, not int64 passing int types around as time values is a bad idea since the timebase/unit/etc are ambiguous. that's why we have typed values
https://codereview.chromium.org/16213002/diff/33002/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/33002/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:120: int64 delta = base::TimeTicks::Now().ToInternalValue() - dispatch_time; On 2013/06/03 18:05:57, jamesr wrote: > you shouldn't do math on internal representations if you can at all help it. in > this case, you can - use a base::TimeDelta to represent the difference between > two times and then pass that to the histogram macro - it understands > base::TimeDelta Sorry, I got confused by your previous comment. Changed back to using TimeTicks. Also, UMA*COUNTS macros do not understand TimeDelta. Changed to UMA*TIMES macro. https://codereview.chromium.org/16213002/diff/33002/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.h (right): https://codereview.chromium.org/16213002/diff/33002/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.h:73: void ForwardToMainListener(const IPC::Message& message, int64 dispatch_time); On 2013/06/03 18:05:57, jamesr wrote: > base::TimeTicks, not int64 passing int types around as time values is a bad idea > since the timebase/unit/etc are ambiguous. that's why we have typed values Done.
lgtm
On 2013/06/01 01:20:23, Rick Byers wrote: > On 2013/06/01 01:19:21, Rick Byers wrote: > > On 2013/05/31 19:36:31, jamesr wrote: > > > > > > https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... > > > File content/renderer/gpu/input_event_filter.cc (right): > > > > > > > > > https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... > > > content/renderer/gpu/input_event_filter.cc:127: > > > base::StringPrintf("Event.Latency.ImplThreadToRendererThread.%s", > > > doing a runtime StringPrintf() every time we hit this codepath is really > > > inefficient when we can only generate one of ~30 strings fixed strings. > please > > > fix > > > > I don't think there's any reason to think the distribution of this histogram > > will change depending on the type of event. I suggest we just keep this > simple > > and use a single event. > > Sorry, I mean a single histogram combined for all event types. On further discussion with Varun, I realize this is incorrect. We should expect a radically different jank profile for, say, MouseMove and GestureScrollUpdate events (since the former doesn't often cause a lot of painting but the latter does). What we want to know here is how much of the jank we're seeing during scrolling is due to other activity on the main thread, vs. time actually spent in the input stack. So I think we do need to at least report GestureScrollUpdate and TouchMove event times individually. Sorry for the back and forth on this. > > > > > > https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... > > > File content/renderer/gpu/input_event_filter.h (right): > > > > > > > > > https://codereview.chromium.org/16213002/diff/13001/content/renderer/gpu/inpu... > > > content/renderer/gpu/input_event_filter.h:74: const base::TimeTicks& > > > dispatch_time); > > > pass TimeTicks by value, not const ref. it's just an int64
On 2013/06/03 18:40:22, jamesr wrote: > lgtm Hi James, Just had a quick discussion with rbyers and I think it is important to have per-event version of this metric (for instance, a bunch of mouse move events that we dont care about will affect the metric greatly in its current form). I will change this patch (will keep your sprintf concern in mind).
On 2013/06/03 19:07:55, varunjain wrote: > On 2013/06/03 18:40:22, jamesr wrote: > > lgtm > > Hi James, Just had a quick discussion with rbyers and I think it is important to > have per-event version of this metric (for instance, a bunch of mouse move > events that we dont care about will affect the metric greatly in its current > form). I will change this patch (will keep your sprintf concern in mind). Added uma metric for individual events. PTAL.
This is still a lot less efficient and clear than it could be. I think this is a symptom of trying to record the histogram in the wrong place (i.e. in a place where we don't otherwise care about what the message is). I think to do this better you'll have to pass the time values around to more places, but that's basically just implementing bits of ui::LatencyInfo. https://codereview.chromium.org/16213002/diff/41003/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/41003/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:22: type_name_map = new EventTypeToUmaNameMap; why the heap allocation? https://codereview.chromium.org/16213002/diff/41003/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:143: const WebInputEvent* event = CrackMessage(message); this is making another copy of the entire message just to get the type enum out. can you figure out a better way?
Hi James, I think the symptom is not so much of trying to record the histogram at the wrong place, as it is of bad coding on my part. We do care about the message (at least the message type) here. One indication is, the re-cracking of the message in the InputEventFilter::SendACK() method. Using LatencyInfo here could actually mean a lot more nastiness because then we would not only need to crack the message, but also modify it with the updated LatencyInfo. I have tried to remedy your message cracking concern in my latest patch. PTAL. https://codereview.chromium.org/16213002/diff/41003/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/41003/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:22: type_name_map = new EventTypeToUmaNameMap; On 2013/06/03 20:51:03, jamesr wrote: > why the heap allocation? Done. https://codereview.chromium.org/16213002/diff/41003/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:143: const WebInputEvent* event = CrackMessage(message); On 2013/06/03 20:51:03, jamesr wrote: > this is making another copy of the entire message just to get the type enum out. > can you figure out a better way? Done.
To jamesr's point, this is why I was thinking that a better direciton for both this and the patch by Rick would be to figure out how to add more stage-based-info into LatencyInfo. The first one you try to plumb through wil be the hardest, but I'm certain it can be done. Once you have it, you have a clean set of data that you can then with ONE SET OF INSTRUMENTATION POINTS share out to uma, telemetry, and about:tracing. Contrast to there being 3 sets of probe points.
https://codereview.chromium.org/16213002/diff/41004/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/41004/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:146: GetPerEventEventLatencyUmaName(event_type), kMin, kMax, 100, somehow this manages to take the task of turning an input type (~5 bits of information, closer to 2 in practice since there only a small number of categories of types) into a pointer to a histogram into an epic ordeal: 1.) hash_map allocation (on first call) 2.) between 2 and 3 hash_map lookups 3.) potentially sprintf(), which involves at least one malloc (more if the buffer has to be resized) and that just gets to a std::string. we then do a runtime histogram lookup on every call which involves taking a mutex and doing an O(lg N) std::map<> lookup on a string-keyed map. then we don't even save the histogram pointer anywhere! the input handling codepaths are fairly warm - not called 1000s of times a second, but potentially in the 100s. we don't have to hand-code this in ASM but we can definitely do better than *this* concretely, i'd recommend picking which input types you care about, hardcoding the histogram strings for those types, then doing something of the form: if (WebKit::WebInputEvent::isTouchEventType(event_type)) { UMA_HISTOGRAM_CUSTOM_TIMES("Event.Latency.ImplThreadToRendererThread.Touch", delta, .... } else if (WebKit::WebInputEvent::isGestureEventType(event_type)) { UMA_HISTOGRAM_CUSTOM_TIMES("Event.Latency.ImplThreadToRendererThread.Gesture", delta, .... } else { // add other categories if you want UMA_HISTOGRAM_CUSTOM_TIMES("Event.Latency.ImplThreadToRendererThread.Other", delta, .... } then you'll have *) No hash_maps *) After first run through for each type, histogram pointer is cached (the macro does that) so you'll have zero allocations, locks, lookups, etc. just incrementing the histogram value if you *really* want a histogram type per input event - which IMO sounds like far more noise than data - use macros to generate the set of strings in the .rodata
@jamesr: Hard-coded specific input types as you suggested and removed the hash map stuff. @rbyers: I have included all GestureScroll* and Touch* event types in UMA. Please have a quick look to see if we need more individual types or these are all we care about. https://codereview.chromium.org/16213002/diff/41004/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/41004/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:146: GetPerEventEventLatencyUmaName(event_type), kMin, kMax, 100, On 2013/06/04 06:44:54, jamesr wrote: > somehow this manages to take the task of turning an input type (~5 bits of > information, closer to 2 in practice since there only a small number of > categories of types) into a pointer to a histogram into an epic ordeal: > > 1.) hash_map allocation (on first call) > 2.) between 2 and 3 hash_map lookups > 3.) potentially sprintf(), which involves at least one malloc (more if the > buffer has to be resized) > > and that just gets to a std::string. we then do a runtime histogram lookup on > every call which involves taking a mutex and doing an O(lg N) std::map<> lookup > on a string-keyed map. then we don't even save the histogram pointer anywhere! > > the input handling codepaths are fairly warm - not called 1000s of times a > second, but potentially in the 100s. we don't have to hand-code this in ASM but > we can definitely do better than *this* > > concretely, i'd recommend picking which input types you care about, hardcoding > the histogram strings for those types, then doing something of the form: > > if (WebKit::WebInputEvent::isTouchEventType(event_type)) { > UMA_HISTOGRAM_CUSTOM_TIMES("Event.Latency.ImplThreadToRendererThread.Touch", > delta, .... > } else if (WebKit::WebInputEvent::isGestureEventType(event_type)) { > UMA_HISTOGRAM_CUSTOM_TIMES("Event.Latency.ImplThreadToRendererThread.Gesture", > delta, .... > } else { // add other categories if you want > UMA_HISTOGRAM_CUSTOM_TIMES("Event.Latency.ImplThreadToRendererThread.Other", > delta, .... > } > > then you'll have > *) No hash_maps > *) After first run through for each type, histogram pointer is cached (the macro > does that) so you'll have zero allocations, locks, lookups, etc. just > incrementing the histogram value > > > if you *really* want a histogram type per input event - which IMO sounds like > far more noise than data - use macros to generate the set of strings in the > .rodata Done. Hard-coded specific input types.
https://codereview.chromium.org/16213002/diff/49001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/49001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:191: "Event.Latency.ImplThreadToRendererThread.GestureScrollBegin", do you *really* need separate histograms for each of these types? what do you expect to learn from the difference in GestureScrollBegin vs GestureScrollUpdate? https://codereview.chromium.org/16213002/diff/49001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.h (right): https://codereview.chromium.org/16213002/diff/49001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.h:97: std::queue<IPC::Message> messages_; you need to rebase, this queue is gone in ToT
https://codereview.chromium.org/16213002/diff/49001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/49001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:191: "Event.Latency.ImplThreadToRendererThread.GestureScrollBegin", On 2013/06/04 17:08:30, jamesr wrote: > do you *really* need separate histograms for each of these types? what do you > expect to learn from the difference in GestureScrollBegin vs > GestureScrollUpdate? Combined these two. Also removed others that we dont need at the moment. https://codereview.chromium.org/16213002/diff/49001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.h (right): https://codereview.chromium.org/16213002/diff/49001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.h:97: std::queue<IPC::Message> messages_; On 2013/06/04 17:08:30, jamesr wrote: > you need to rebase, this queue is gone in ToT Done.
> @rbyers: I have included all GestureScroll* and Touch* event types in UMA. > Please have a quick look to see if we need more individual > types or these are all we care about. Looks reasonable to me, I have a couple minor suggestions. https://codereview.chromium.org/16213002/diff/57001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/57001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:159: UMA_HISTOGRAM_CUSTOM_TIMES("Event.Latency.ImplThreadToRendererThread", I'm not sure this adds much value once we have the two below. https://codereview.chromium.org/16213002/diff/57001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:163: case WebInputEvent::GestureScrollUpdate: I would add GestureScrollUpdateWithoutPropagation here (I'm not sure offhand where we add that, but even if it can't reach here right now we should still include it for consistency I think). You could also argue for GestureFlingStart - we probably care about jank for it just as much as for the others, but there may be no good reason to expect it to have different results than update I think... https://codereview.chromium.org/16213002/diff/57001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:164: UMA_HISTOGRAM_CUSTOM_TIMES( You could make this code even simpler by adding UMA_HISTOGRAM_TIMES_SHORT. I think microsecond-level timing is common enough to deserve it's own macro.
https://codereview.chromium.org/16213002/diff/57001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/57001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:159: UMA_HISTOGRAM_CUSTOM_TIMES("Event.Latency.ImplThreadToRendererThread", On 2013/06/04 20:19:36, Rick Byers wrote: > I'm not sure this adds much value once we have the two below. Done. https://codereview.chromium.org/16213002/diff/57001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:163: case WebInputEvent::GestureScrollUpdate: On 2013/06/04 20:19:36, Rick Byers wrote: > I would add GestureScrollUpdateWithoutPropagation here (I'm not sure offhand > where we add that, but even if it can't reach here right now we should still > include it for consistency I think). > > You could also argue for GestureFlingStart - we probably care about jank for it > just as much as for the others, but there may be no good reason to expect it to > have different results than update I think... Done. https://codereview.chromium.org/16213002/diff/57001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:164: UMA_HISTOGRAM_CUSTOM_TIMES( On 2013/06/04 20:19:36, Rick Byers wrote: > You could make this code even simpler by adding UMA_HISTOGRAM_TIMES_SHORT. I > think microsecond-level timing is common enough to deserve it's own macro. Done.
Someone feel like responding to my comments?
On 2013/06/04 21:12:49, nduca wrote: > Someone feel like responding to my comments? Hi Nat, Sorry for the lack of response. I was hoping that Rick's reply to the separate email discussion had appeased your concerns. In general, I am not opposed to the use of LatencyInfo. But I'd prefer not to use it for this particular change because of the following concerns: 1. Performance: As James's review of this patch has revealed, we really care about performance here so much so that, even cracking the message again was not preferred. So how do we justify the use of LatencyInfo (which will require modifying the IPC message to update the Info.. or is there a better way of doing this that I do not know?). 2. LatencyInfo is not needed for this metric as no downstream code will need these times and we have everything we need right here to calculate this particular latency. Using it for this metric will add complexity without much gain IMHO. Re-iterating what Rick said in the email thread: if we do need to let any timings flow through the system, we will use LatencyInfo and add/modify to it as appropriate.
On 2013/06/04 21:48:17, varunjain wrote: > On 2013/06/04 21:12:49, nduca wrote: > > Someone feel like responding to my comments? > > Hi Nat, Sorry for the lack of response. I was hoping that Rick's reply to the > separate email discussion had appeased your concerns. In general, I am not > opposed to the use of LatencyInfo. But I'd prefer not to use it for this > particular change because of the following concerns: > 1. Performance: As James's review of this patch has revealed, we really care > about performance here so much so that, even cracking the message again was not > preferred. So how do we justify the use of LatencyInfo (which will require > modifying the IPC message to update the Info.. or is there a better way of doing > this that I do not know?). Using LatencyInfo should not cause any performance issues, certainly not any comparable to the ones earlier versions of this patch had. > 2. LatencyInfo is not needed for this metric as no downstream code will need > these times and we have everything we need right here to calculate this > particular latency. Using it for this metric will add complexity without much > gain IMHO. Re-iterating what Rick said in the email thread: if we do need to let > any timings flow through the system, we will use LatencyInfo and add/modify to > it as appropriate. I don't think that's really true - downstream code is definitely going to want to know the timing information from various stages of the event pipeline.
Sorry, Ricks' reply didn't help me. I would like to hear how we get this information into LatencyInfo, and then discuss how to get that into UMA. There are many consumers of this data --- uma, telemetry benchmarks, about:tracing. We should have **one** collection approach that can feed many consumers. LatencyINfo is that system.
This seems like a reasonable approach to me - track the key timestamps in LatencyInfo, but immediately report an UMA histogram for the delta once we have it (so we don't need to worry about some later coalescing. It would probably also be useful to report the average value of this delta up through the smoothness_measurement tests (as Yufeng has done for his other metrics here: https://codereview.chromium.org/17757002/) Nat, does this look good to you now? https://codereview.chromium.org/16213002/diff/67001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/67001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:148: latency_info.MergeWith(NewLatencyInfo(message, Can you just call AddNumber directly? What does creating a new LatencyInfo and merging it in buy you? https://codereview.chromium.org/16213002/diff/67001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:217: const base::TimeTicks& dispatch_time = latency_info.latency_components.at( A LatencyInfo::Lookup helper would be cleaner (just suggested the same thing to miletus in his CL where he does this slightly differently).
I have added telemetry reporting as well for this metric (a bit incomplete as I will need to wait for miletus' patch to land). https://codereview.chromium.org/16213002/diff/67001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/67001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:148: latency_info.MergeWith(NewLatencyInfo(message, On 2013/06/26 17:21:57, Rick Byers wrote: > Can you just call AddNumber directly? What does creating a new LatencyInfo and > merging it in buy you? Done. https://codereview.chromium.org/16213002/diff/67001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:217: const base::TimeTicks& dispatch_time = latency_info.latency_components.at( On 2013/06/26 17:21:57, Rick Byers wrote: > A LatencyInfo::Lookup helper would be cleaner (just suggested the same thing to > miletus in his CL where he does this slightly differently). I will wait for that patch to land
ping @jamesr and nduca: I have switched to using latency info for this metric. PTAL. On 2013/06/27 17:29:56, varunjain wrote: > I have added telemetry reporting as well for this metric (a bit incomplete as I > will need to wait for miletus' patch to land). > > https://codereview.chromium.org/16213002/diff/67001/content/renderer/gpu/inpu... > File content/renderer/gpu/input_event_filter.cc (right): > > https://codereview.chromium.org/16213002/diff/67001/content/renderer/gpu/inpu... > content/renderer/gpu/input_event_filter.cc:148: > latency_info.MergeWith(NewLatencyInfo(message, > On 2013/06/26 17:21:57, Rick Byers wrote: > > Can you just call AddNumber directly? What does creating a new LatencyInfo > and > > merging it in buy you? > > Done. > > https://codereview.chromium.org/16213002/diff/67001/content/renderer/gpu/inpu... > content/renderer/gpu/input_event_filter.cc:217: const base::TimeTicks& > dispatch_time = latency_info.latency_components.at( > On 2013/06/26 17:21:57, Rick Byers wrote: > > A LatencyInfo::Lookup helper would be cleaner (just suggested the same thing > to > > miletus in his CL where he does this slightly differently). > > I will wait for that patch to land
https://codereview.chromium.org/16213002/diff/81001/content/renderer/render_w... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/16213002/diff/81001/content/renderer/render_w... content/renderer/render_widget.cc:187: receive_component.event_time - dispatch_component.event_time; This won't calculate an average. You'd need to sum up the differences across multiple events and keep a counter of events as well, and divide in the python script. https://codereview.chromium.org/16213002/diff/81001/content/renderer/render_w... content/renderer/render_widget.cc:791: UpdateRenderingStatsForAverageImplThreadLatency(routing_id(), latency_info_, You probably don't want to use latency_info_ here. It won't be updated in the accelerated compositing case.
https://codereview.chromium.org/16213002/diff/81001/content/renderer/render_w... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/16213002/diff/81001/content/renderer/render_w... content/renderer/render_widget.cc:187: receive_component.event_time - dispatch_component.event_time; On 2013/07/03 20:00:15, jbauman wrote: > This won't calculate an average. You'd need to sum up the differences across > multiple events and keep a counter of events as well, and divide in the python > script. Done. https://codereview.chromium.org/16213002/diff/81001/content/renderer/render_w... content/renderer/render_widget.cc:791: UpdateRenderingStatsForAverageImplThreadLatency(routing_id(), latency_info_, On 2013/07/03 20:00:15, jbauman wrote: > You probably don't want to use latency_info_ here. It won't be updated in the > accelerated compositing case. Done.
this still feels pretty wrong to me https://codereview.chromium.org/16213002/diff/96001/cc/debug/rendering_stats.h File cc/debug/rendering_stats.h (right): https://codereview.chromium.org/16213002/diff/96001/cc/debug/rendering_stats.... cc/debug/rendering_stats.h:40: int64 total_impl_thread_to_main_thread_count; these variable names make no sense. impl_thread_to_main_thread_whats? total_somethings_sent_from_impl_to_main_thread or num same goes for the time... https://codereview.chromium.org/16213002/diff/96001/content/renderer/gpu/inpu... File content/renderer/gpu/input_event_filter.cc (right): https://codereview.chromium.org/16213002/diff/96001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:168: ui::INPUT_EVENT_LATENCY_SENT_FROM_IMPL_THREAD_COMPONENT, is this enum clearly named? https://codereview.chromium.org/16213002/diff/96001/content/renderer/gpu/inpu... content/renderer/gpu/input_event_filter.cc:169: message.routing_id(), 0); why are you passing zero here? that seems like abuse of the latency struct... https://codereview.chromium.org/16213002/diff/96001/content/renderer/render_w... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/16213002/diff/96001/content/renderer/render_w... content/renderer/render_widget.cc:143: void SendToImplThreadLatencyUma(int routing_id, see you're just using the latency info struct but you'er still doing the probe here. your uma should be up in the browser process where you're tracking things about the input events as they're acked by the host process. the renderer really shouldn't be doing anything.
https://codereview.chromium.org/16213002/diff/96001/content/renderer/render_w... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/16213002/diff/96001/content/renderer/render_w... content/renderer/render_widget.cc:143: void SendToImplThreadLatencyUma(int routing_id, On 2013/07/08 20:58:32, nduca wrote: > see you're just using the latency info struct but you'er still doing the probe > here. > > your uma should be up in the browser process where you're tracking things about > the input events as they're acked by the host process. the renderer really > shouldn't be doing anything. Hi Nat, while I am looking into how to plumb the latency info back into the browser, I wanted to ask why is it wrong to do this in the renderer? Especially because this metric measures latency that exists entirely in the renderer. We are recording other UMA stats in render_widget, so why is this one not allowed?
The latency info struct is in a partial state when you're in the browser. Only at the point of FrameSwapped in browser do you have all the latency info that was collected for the frame. Your approach would uma your metric but you're not thinking big picture enough (still!). Supopse we wanted to uma the renderer->gpu latency. Or the input to screen latency. By umaing in the host, you get that. By doing it the renderer main, you dont.
On 2013/07/11 01:22:14, nduca wrote: > The latency info struct is in a partial state when you're in the browser. Only > at the point of FrameSwapped in browser do you have all the latency info that > was collected for the frame. You don't actually. You have times that certain events happened, but you don't have pairwise latencies. > > Your approach would uma your metric but you're not thinking big picture enough > (still!). Supopse we wanted to uma the renderer->gpu latency. Or the input to > screen latency. By umaing in the host, you get that. By doing it the renderer > main, you dont. If you wanted renderer->GPU latency, you could calculate that in the GPU process. Input to screen latency you could calculate in FrameSwapped.
> > Your approach would uma your metric but you're not thinking big picture enough > > (still!). Supopse we wanted to uma the renderer->gpu latency. Or the input to > > screen latency. By umaing in the host, you get that. By doing it the renderer > > main, you dont. > > If you wanted renderer->GPU latency, you could calculate that in the GPU > process. IMO, you'd put the right fields into the struct, but report it in the browser.
On 2013/07/11 05:15:23, nduca wrote: > > > Your approach would uma your metric but you're not thinking big picture > enough > > > (still!). Supopse we wanted to uma the renderer->gpu latency. Or the input > to > > > screen latency. By umaing in the host, you get that. By doing it the > renderer > > > main, you dont. > > > > If you wanted renderer->GPU latency, you could calculate that in the GPU > > process. > IMO, you'd put the right fields into the struct, but report it in the browser. We had a separate thread about this where we discussed that either LatencyInfo needs to carry full pairwise information for all the events that went into it (eg. carry a tree around of potentially unbounded size, or a full histogram), or when we were concerned with the latency at some intermediate step we could just record that value in place (for both UMA and browser rendering stats). If I recall, the conclusion of that thread was that it wasn't worth the complexity and additional cost of carrying this unbounded information all the way to FrameSwapped time, and we should just record our aggregate stats in the middle of the flow somewhere. For a general purpose latency tracking system, I don't see why FrameSwapped time should be so special that it's the only time at which statistics can be computed. Eg. what if we decide we want to track the latency of a higher level action, like from the time the user clicks on a link to when the page has redrawn. Would we then say that since some LatencyInfo object can survive past FrameSwapped (and since a LatencyInfo struct isn't really 'complete' at frame swapped time), that this new endpoint (where potentially hundreds of frames have been merged) is the only time at which we can record any stats? It's much more practical to decompose the problem into regions of interest - you and the GPU team can be primarily concerned with regions of time that end in frame swapped, while me and my team are primarily interested in regions that end with handing events off to the compositor or webkit (and potentially others can be concerned with regions that span multiple frames and end in events like page load). If we want to use a common latency tracking infrastructure for all latency measurements, it shouldn't be biased towards frame swap as the one privileged end point.
You're putting the tracking in one place so that we can expose this to telemetry as well. You can't do that if you're reporting the latencies piecemeal in the different stages of the pipeline. I would like telemetry to be able to report out the stage wise latencies of the system as it runs through each page set. That gives you the basic option: will this info come in on the BrowserRenderStats, GpuRenderStats, or RenderWidgetRenderStats. The reason this patch continues to flounder is because it is monofocused on the UMA case. If you would work more on the use case above, of showing the piecewise breakdown within telemetry, than you'd encounter less resistance on your patch because you'd be making more decisions in a direction that was favorable to that use case.
Hi Nat, John, I would like to restart review for this patch. As we discussed in offline meeting, I no longer collect this metric in UMA. Instead, it is reported to telemetry (changed the title of the patch accordingly). PTAL. On 2013/07/15 21:49:33, nduca wrote: > You're putting the tracking in one place so that we can expose this to telemetry > as well. You can't do that if you're reporting the latencies piecemeal in the > different stages of the pipeline. I would like telemetry to be able to report > out the stage wise latencies of the system as it runs through each page set. > That gives you the basic option: will this info come in on the > BrowserRenderStats, GpuRenderStats, or RenderWidgetRenderStats. > > The reason this patch continues to flounder is because it is monofocused on the > UMA case. If you would work more on the use case above, of showing the piecewise > breakdown within telemetry, than you'd encounter less resistance on your patch > because you'd be making more decisions in a direction that was favorable to that > use case.
Thanks Varun. I've got a lot of feedback. You might want to consider a strategy of a few different patches based on this feedback, otherwise the patch is going to be so large that I'm going to avoid reviewing it because its big and scary. Assuming my suggestions themselves aren't too objectionable, I suggest: 1. one to possibly change the host side of things so that latency is computed inside the input router rather than outside 2. one to condition the existing codebase for better unit testing of this eventual feature 3. one to change the way input events are stored before the composite is called so you avoid the const cast hack and fix a known bug while you're at it. 4. one patch to add the actual latencyinfo bookkeeping calls 5. a patch that exposes the new latencyinfo to telemetry I know this is a lot. Is there anything I should be changing in my worldview so that I can help relieve pressure on you such that we do the right thing in the long term but dont land you in a pile of pain in the near term? https://codereview.chromium.org/16213002/diff/106001/content/browser/renderer... File content/browser/renderer_host/input/immediate_input_router.h (right): https://codereview.chromium.org/16213002/diff/106001/content/browser/renderer... content/browser/renderer_host/input/immediate_input_router.h:123: InputEventAckState ack_result, here you're changing just the gesture ack but not the others. This is a sign to me of something not fully thought out... why gesture but not anything else? https://codereview.chromium.org/16213002/diff/106001/content/browser/renderer... File content/browser/renderer_host/input/immediate_input_router_unittest.cc (left): https://codereview.chromium.org/16213002/diff/106001/content/browser/renderer... content/browser/renderer_host/input/immediate_input_router_unittest.cc:3: // found in the LICENSE file. no unit test coverage for this new feature? how would we test the full stack? [hint: do a patch that pulls the render_widget parts of the input receipt code out, plus ideally the compositor side things, and make it into a class. then build a test that instantiates input router plus the child input handler classes and then verifies that when you inject an input event, then do the right things on the child input classes, you get the right calls back up on the client.] Yes you're getting the bill for adding tests that should have been there for ages. Sorry! Complain to your local TL for letting the stack get to the point that there's so little test coverage in the first place. ^_^ This will be good though! :) Happy to talk to you offline if the concept here doesn't make sense. Think: instead of testing this by instantiating real processes, instantitate the classes that would be existing, mock out the IPC parts of things, and then poke at the classes manually. https://codereview.chromium.org/16213002/diff/106001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/16213002/diff/106001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:2369: void RenderWidgetHostImpl::UpdateRenderingStatsForImplThreadLatency( I'd like to see you modify enough of the input router etc so that InputRouter is able to track stats. E.g. every input event that you give to the input router comes back with latency info. Can you think of a way to change the host side interfaces to permit this? https://codereview.chromium.org/16213002/diff/106001/content/renderer/render_... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/16213002/diff/106001/content/renderer/render_... content/renderer/render_widget.cc:791: const_cast<ui::LatencyInfo&>(latency_info).AddLatencyNumber( hmmm remember how we talked about fixing the way input events are tracked wrt frame generation? i think the fact that youre doing a const cast here to get rid of the const is a sign of needing to do that cleanup first. it seems better that we stashing the input event here, once you have that, it'll be easy to come back here and add this call without a const çast.
varunjain@: Please see crrev.com/23137002 for some additional thoughts I had on this kind of stats reporting. In short, we should not be adding more "UpdateFooStats" methods to RWHI: please attach such methods directly to BrowserRenderingStats. This will make it much easier to move the stats object around should we so desire, e.g., into the InputRouter.
On 2013/08/15 01:41:03, nduca wrote: > Thanks Varun. I've got a lot of feedback. You might want to consider a strategy > of a few different patches based on this feedback, otherwise the patch is going > to be so large that I'm going to avoid reviewing it because its big and scary. > > Assuming my suggestions themselves aren't too objectionable, I suggest: > 1. one to possibly change the host side of things so that latency is computed > inside the input router rather than outside > 2. one to condition the existing codebase for better unit testing of this > eventual feature > 3. one to change the way input events are stored before the composite is called > so you avoid the const cast hack and fix a known bug while you're at it. > 4. one patch to add the actual latencyinfo bookkeeping calls > 5. a patch that exposes the new latencyinfo to telemetry > > I know this is a lot. Is there anything I should be changing in my worldview so > that I can help relieve pressure on you such that we do the right thing in the > long term but dont land you in a pile of pain in the near term? > > https://codereview.chromium.org/16213002/diff/106001/content/browser/renderer... > File content/browser/renderer_host/input/immediate_input_router.h (right): > > https://codereview.chromium.org/16213002/diff/106001/content/browser/renderer... > content/browser/renderer_host/input/immediate_input_router.h:123: > InputEventAckState ack_result, > here you're changing just the gesture ack but not the others. This is a sign to > me of something not fully thought out... why gesture but not anything else? > > https://codereview.chromium.org/16213002/diff/106001/content/browser/renderer... > File content/browser/renderer_host/input/immediate_input_router_unittest.cc > (left): > > https://codereview.chromium.org/16213002/diff/106001/content/browser/renderer... > content/browser/renderer_host/input/immediate_input_router_unittest.cc:3: // > found in the LICENSE file. > no unit test coverage for this new feature? how would we test the full stack? > > > [hint: do a patch that pulls the render_widget parts of the input receipt code > out, plus ideally the compositor side things, and make it into a class. then > build a test that instantiates input router plus the child input handler classes > and then verifies that when you inject an input event, then do the right things > on the child input classes, you get the right calls back up on the client.] > > Yes you're getting the bill for adding tests that should have been there for > ages. Sorry! Complain to your local TL for letting the stack get to the point > that there's so little test coverage in the first place. ^_^ This will be good > though! :) > > Happy to talk to you offline if the concept here doesn't make sense. Think: > instead of testing this by instantiating real processes, instantitate the > classes that would be existing, mock out the IPC parts of things, and then poke > at the classes manually. > > https://codereview.chromium.org/16213002/diff/106001/content/browser/renderer... > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > https://codereview.chromium.org/16213002/diff/106001/content/browser/renderer... > content/browser/renderer_host/render_widget_host_impl.cc:2369: void > RenderWidgetHostImpl::UpdateRenderingStatsForImplThreadLatency( > I'd like to see you modify enough of the input router etc so that InputRouter is > able to track stats. E.g. every input event that you give to the input router > comes back with latency info. Can you think of a way to change the host side > interfaces to permit this? > > https://codereview.chromium.org/16213002/diff/106001/content/renderer/render_... > File content/renderer/render_widget.cc (right): > > https://codereview.chromium.org/16213002/diff/106001/content/renderer/render_... > content/renderer/render_widget.cc:791: > const_cast<ui::LatencyInfo&>(latency_info).AddLatencyNumber( > hmmm remember how we talked about fixing the way input events are tracked wrt > frame generation? > Regarding this, can you share your thoughts at crbug.com/271583 on jamesr@'s comments ? > i think the fact that youre doing a const cast here to get rid of the const is a > sign of needing to do that cleanup first. it seems better that we stashing the > input event here, > > once you have that, it'll be easy to come back here and add this call without a > const çast.
To recap our discussion offline: 1. Its cool to use a browser test for this. Goood point, sorry I missed that! 2. We can attack the input<->composited frame coorelation as a followup SOrry for the confusion! |
