Issue 228320043: Fix input latency side panel issue
|
Created:
9 years ago by miletus Modified:
8 years, 11 months ago CC:
trace-viewer-review_googlegroups.com Base URL:
https://github.com/google/trace-viewer.git@master Visibility:
Public. |
DescriptionFix input latency side panel issue
Input latency side panel and RAIL analysis code should
only work on input latency trace event, not non-input
latency trace event. The filtering logic is that, in
the future only input latency trace event's name has prefix
"InputLatency".
Currently, some non-input related latency trace event also
have name "InputLatencyXXX". To filter these trace events
out, we check if the LatencyInfo has BEGIN_RWH component
(only input latency has this component).
BUG=#828
Patch Set 1 #Patch Set 2 : filter out InputLatency that does not have BEGIN_RWH component #
Total comments: 3
Patch Set 3 : rework based on recently refactor of nput_latency_async_slice.html #Patch Set 4 : #
Total comments: 7
Patch Set 5 : address comments #
Total comments: 1
Patch Set 6 : rework the test #Patch Set 7 : fixed typo #
MessagesTotal messages: 23
that is very confusing, why is it a latency info event in the first place?
Sign in to reply to this message.
On 2015/04/17 15:56:39, nduca wrote: > that is very confusing, why is it a latency info event in the first place? Originally input latency tracking is just for input events that originates in browser process. Later we wanted to track the scroll effect slippage between impl and main (scroll happens in impl, but onscroll listener on main can also generate damage and the damage might not go into the same frame that the impl generates for the scroll event). So we created this "ScrollUpdate" latency tracking, that starts when the scroll update information is forwarded from impl to main to feed into onscroll listener, and ends when the main frame hits screen.
Sign in to reply to this message.
then why is it called a latency event?
Sign in to reply to this message.
On 2015/04/17 16:08:32, nduca wrote: > then why is it called a latency event? well it is still latency tracking, just covers a different path for our threaded scrolling model: one covers the latency of compositor scrolling and the other covers the latency of scroll linked effect.
Sign in to reply to this message.
how do we make this more understandable to users? we say to users that latency info show input activity but they dont always i am learning. and its only encoded in a few people's mind when it doesn't. this isn't so great. please try to step back from the details and try to put on your "making it easy for other people to work with you" hat. whats the right course?
Sign in to reply to this message.
On 2015/04/17 16:28:57, nduca wrote: > how do we make this more understandable to users? > > we say to users that latency info show input activity > > but they dont always i am learning. > > and its only encoded in a few people's mind when it doesn't. > > this isn't so great. please try to step back from the details and try to put on > your "making it easy for other people to work with you" hat. whats the right > course? I think we can change the trace name from "InputLatency:ScrollUpdate" to "Latency:ScrollUpdate". Then: 1) telemetry metrics part: we still want to compute the metrics, but now searching for "Latency:ScrollUpdate" trace events. 2) trace viewer: people not so familiar with InputLatncy details won't get confused by the new name, and people knowing the details can still find this latency tracking useful 3) RAIL auditing code: don't expose this latency data to auditing code at all. does this sound like it could make it less confusing ?
Sign in to reply to this message.
Having a "hierarchy" of latencies sounds like a good way to think about this. We only want to show some types of latency metrics to users (e.g., InputLatency) but still all the statistics can be collected by the same underlying tracking mechanism.
Sign in to reply to this message.
This sounds promising. To be clear, you're saying changing the name of the async event right? RAIL analysis code is typically of the form "find all input latency records"... I'd like finding those to remain pretty straightforward.
Sign in to reply to this message.
in the meantime, please update these methods to be backcompat: there is code here asking for input latency events, separate from others. Maybe you can have a getInputLatencyEventsInRange(...) function?
Sign in to reply to this message.
On 2015/04/17 18:25:53, nduca wrote:
> This sounds promising. To be clear, you're saying changing the name of the
async
> event right?
>
> RAIL analysis code is typically of the form "find all input latency
records"...
> I'd like finding those to remain pretty straightforward.
Yes, the name of the async event.
RAIL code to find the input latency records is just
event.title.indexOf('InputLatency') === 0
so it will automatically filter out "Latency:ScrollUpdate".
Sign in to reply to this message.
can't these just be differently named events? ScrollUpdateLatency? InputLatency? Also, why are we doing single colon? The convention in the rest of the ecosystem seems to be dotting to denote hierarchy and :: for namespacing.
Sign in to reply to this message.
On 2015/04/17 18:39:34, nduca wrote: > can't these just be differently named events? ScrollUpdateLatency? InputLatency? > > > Also, why are we doing single colon? The convention in the rest of the ecosystem > seems to be dotting to denote hierarchy and :: for namespacing. Alright, I will take the naming of "InputLatency::XXX" and "Latency::ScrollUpdate". Showing the actual event type in the trace event's name is useful. To address the backcompat issue you mentioned above, I added an existence check for BEGIN_RWH component, which only exists for input latency that originates from RenderWidgetHost from browser process, but not in ScrollUpdate latency. PTAL, thanks.
Sign in to reply to this message.
On 2015/04/17 18:39:34, nduca wrote: > can't these just be differently named events? ScrollUpdateLatency? InputLatency? > > > Also, why are we doing single colon? The convention in the rest of the ecosystem > seems to be dotting to denote hierarchy and :: for namespacing. Alright, I will take the naming of "InputLatency::XXX" and "Latency::ScrollUpdate". Showing the actual event type in the trace event's name is useful. To address the backcompat issue you mentioned above, I added an existence check for BEGIN_RWH component, which only exists for input latency that originates from RenderWidgetHost from browser process, but not in ScrollUpdate latency. PTAL, thanks.
Sign in to reply to this message.
lgtm.
Sign in to reply to this message.
not ready to land yet, sorry. please focus more on making this code easy to understandable. also please make sure you aren't regressing rail_ir_finder https://codereview.appspot.com/228320043/diff/20001/trace_viewer/extras/audit... File trace_viewer/extras/audits/chrome_browser_helper.html (right): https://codereview.appspot.com/228320043/diff/20001/trace_viewer/extras/audit... trace_viewer/extras/audits/chrome_browser_helper.html:58: if (!('data' in event.args) || !(BEGIN_COMP_NAME in event.args.data)) Please put a comment here documenting what it is doing. I don't feel you're doing enough yet to make this code understandable to drive by readers of the code. https://codereview.appspot.com/228320043/diff/20001/trace_viewer/extras/audit... trace_viewer/extras/audits/chrome_browser_helper.html:58: if (!('data' in event.args) || !(BEGIN_COMP_NAME in event.args.data)) and why would data not be in event.args? if there are separate checks here, they should be separate if statements with an explainer on each. https://codereview.appspot.com/228320043/diff/20001/trace_viewer/extras/audit... File trace_viewer/extras/audits/chrome_model_helper_test.html (right): https://codereview.appspot.com/228320043/diff/20001/trace_viewer/extras/audit... trace_viewer/extras/audits/chrome_model_helper_test.html:31: events.push({'cat' : 'benchmark', 'pid' : 3507, 'tid': 3507, 'ts' : end_ts, 'ph' : 'F', 'name' : 'InputLatency', 'args' : {'data' : {'INPUT_EVENT_LATENCY_ORIGINAL_COMPONENT' : {'time' : start_ts}, 'INPUT_EVENT_LATENCY_BEGIN_RWH_COMPONENT' : {'time' : start_ts}, 'INPUT_EVENT_LATENCY_TERMINATED_FRAME_SWAP_COMPONENT' : {'time' : end_ts}}}, 'id' : i}); // @suppress longLineCheck this should be a new test case that explains what it is testing. you can't keep adding more complexity to same unit test. i have no idea what this test is testing at this point.
Sign in to reply to this message.
to the tests, i'd like there to be a variety of tests that show the different scenarios so that its clear in the code what kinds of input latency events show up in the wild. there're lots of conditionals in the latency finding code, and it'd be good if it was clear to see the different kinds of events that happen in practice by skimming the tests. for instance, a line like "if (event.args.data===undefined)" should come with a comment on why its there, and a test case that shows it.
Sign in to reply to this message.
Sorry for letting it sitting for too long. Nat, I have rebased the change based on your rework of input_latency_async_slice.html. It seems much simpler. PTAL, thanks.
Sign in to reply to this message.
https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/audit... File trace_viewer/extras/audits/chrome_auditor_test.html (right): https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/audit... trace_viewer/extras/audits/chrome_auditor_test.html:33: why no tests here? https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/side_... File trace_viewer/extras/side_panel/input_latency.html (right): https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/side_... trace_viewer/extras/side_panel/input_latency.html:243: latencyData.sort(function(x, y) { this seems like a separate patch https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/side_... File trace_viewer/extras/side_panel/input_latency_test.html (right): https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/side_... trace_viewer/extras/side_panel/input_latency_test.html:65: // old style of InputLatency slice. capital O https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/side_... trace_viewer/extras/side_panel/input_latency_test.html:112: // new style of InputLatency slice. this feels very dirty. why would the side panel need two tests? the job of the input latency event is now to normalize the events. the side panel shouldn't work with the normalized data. please make sure the latency event has old and new style tests.
Sign in to reply to this message.
https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/audit... File trace_viewer/extras/audits/chrome_auditor_test.html (right): https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/audit... trace_viewer/extras/audits/chrome_auditor_test.html:33: On 2015/05/15 17:57:34, nduca wrote: > why no tests here? Similar here, input_latency_async_slice_test.html should be the place to cover the handling of old/new style format, and we probably should just leave this test as is ? https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/side_... File trace_viewer/extras/side_panel/input_latency.html (right): https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/side_... trace_viewer/extras/side_panel/input_latency.html:243: latencyData.sort(function(x, y) { On 2015/05/15 17:57:35, nduca wrote: > this seems like a separate patch removed. https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/side_... File trace_viewer/extras/side_panel/input_latency_test.html (right): https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/side_... trace_viewer/extras/side_panel/input_latency_test.html:112: // new style of InputLatency slice. On 2015/05/15 17:57:35, nduca wrote: > this feels very dirty. > > why would the side panel need two tests? > > the job of the input latency event is now to normalize the events. the side > panel shouldn't work with the normalized data. > > please make sure the latency event has old and new style tests. Right, I think input_latency_async_slice_test.html should be the place to cover the old/new style format, while this test should focus on the brushedRangeChange event.
Sign in to reply to this message.
On 2015/05/19 20:16:03, miletus wrote: > https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/audit... > File trace_viewer/extras/audits/chrome_auditor_test.html (right): > > https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/audit... > trace_viewer/extras/audits/chrome_auditor_test.html:33: > On 2015/05/15 17:57:34, nduca wrote: > > why no tests here? > > Similar here, input_latency_async_slice_test.html should be the place to cover > the handling of old/new style format, and we probably should just leave this > test as is ? > > https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/side_... > File trace_viewer/extras/side_panel/input_latency.html (right): > > https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/side_... > trace_viewer/extras/side_panel/input_latency.html:243: > latencyData.sort(function(x, y) { > On 2015/05/15 17:57:35, nduca wrote: > > this seems like a separate patch > > removed. > > https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/side_... > File trace_viewer/extras/side_panel/input_latency_test.html (right): > > https://codereview.appspot.com/228320043/diff/60001/trace_viewer/extras/side_... > trace_viewer/extras/side_panel/input_latency_test.html:112: // new style of > InputLatency slice. > On 2015/05/15 17:57:35, nduca wrote: > > this feels very dirty. > > > > why would the side panel need two tests? > > > > the job of the input latency event is now to normalize the events. the side > > panel shouldn't work with the normalized data. > > > > please make sure the latency event has old and new style tests. > > Right, I think input_latency_async_slice_test.html should be the > place to cover the old/new style format, while this test should focus on the > brushedRangeChange event. ping.
Sign in to reply to this message.
https://codereview.appspot.com/228320043/diff/80001/trace_viewer/extras/cc/in... File trace_viewer/extras/cc/input_latency_async_slice_test.html (right): https://codereview.appspot.com/228320043/diff/80001/trace_viewer/extras/cc/in... trace_viewer/extras/cc/input_latency_async_slice_test.html:41: var sInner = newAsyncSliceEx({ tests that are monolithic --- one giant scenario followed by a pile of assertions --- are incredibly hard to maintain over time. please avoid this. in this case, you are augmenting a test that is about matchingByType. whereas, you've really got two tests: first test , makes an input latency async slice with valid data and checks the computed latency second test, you're also testing the legacy format. why? your test data should be inline, not as a constant at the top.
Sign in to reply to this message.
On 2015/05/27 04:48:25, nduca wrote: > https://codereview.appspot.com/228320043/diff/80001/trace_viewer/extras/cc/in... > File trace_viewer/extras/cc/input_latency_async_slice_test.html (right): > > https://codereview.appspot.com/228320043/diff/80001/trace_viewer/extras/cc/in... > trace_viewer/extras/cc/input_latency_async_slice_test.html:41: var sInner = > newAsyncSliceEx({ > tests that are monolithic --- one giant scenario followed by a pile of > assertions --- are incredibly hard to maintain over time. please avoid this. > > in this case, you are augmenting a test that is about matchingByType. > > whereas, you've really got two tests: > first test , makes an input latency async slice with valid data and checks the > computed latency > second test, > > you're also testing the legacy format. why? > > your test data should be inline, not as a constant at the top. I made the existing matchingXXX tests to have valid data, so their focus is still on the matching. I added another test with invalid data, so it is specifically testing the data format. PTAL, thanks.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
