Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1811833002:
Show max latency on client's status bar (Closed)
DescriptionShow max latency on client's status bar
Will later also consider storing max latency data in server
BUG=560950
Committed: https://crrev.com/e37a467453f3814a9ecb0eb6769bd7c58b61cbae
Cr-Commit-Position: refs/heads/master@{#382663}
Committed: https://crrev.com/d2a0c5cb1217570e7b3da52415f7326f48928053
Cr-Commit-Position: refs/heads/master@{#382957}
Patch Set 1 #
Total comments: 12
Patch Set 2 : #
Total comments: 25
Patch Set 3 : [Remoting WebApp]Shows max capture/encode/render/roundtrip latency #Patch Set 4 : Client code for uploading stats to telemetry server #
Total comments: 9
Patch Set 5 : [Remoting WebApp]Show and upload max capture/encode/render/roundtrip latency #Patch Set 6 : [Remoting WebApp]Show and upload max capture/encode/render/roundtrip latency #
Total comments: 1
Patch Set 7 : [Remoting WebApp]Show and upload max capture/encode/render/roundtrip latency #Patch Set 8 : [Remoting WebApp]Show and upload max capture/encode/render/roundtrip latency #
Total comments: 1
Patch Set 9 : JS compiler fix and lld fix #
Messages
Total messages: 72 (28 generated)
yuweih@chromium.org changed reviewers: + sergeyu@chromium.org
Hi, this is the CL for showing maximum latency on the client webapp. PTAL:)
https://codereview.chromium.org/1811833002/diff/1/remoting/base/running_max.cc File remoting/base/running_max.cc (right): https://codereview.chromium.org/1811833002/diff/1/remoting/base/running_max.c... remoting/base/running_max.cc:5: #include "remoting/base/running_max.h" running_max.h and running_max.cc are copied and modified from running_average.*. Looks like I should have written unit test for this. Will figure out how to do so.
https://codereview.chromium.org/1811833002/diff/1/remoting/base/running_max.h File remoting/base/running_max.h (right): https://codereview.chromium.org/1811833002/diff/1/remoting/base/running_max.h... remoting/base/running_max.h:21: class RunningMax { I don't think we really need both RunningAverage class and RunningMax. Instead it would be better to have a single class that host last N samples for the value and can return both average and max. Just add Max() in RunningAverage, and maybe rename that class? https://codereview.chromium.org/1811833002/diff/1/remoting/client/plugin/chro... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1811833002/diff/1/remoting/client/plugin/chro... remoting/client/plugin/chromoting_instance.cc:1040: data->SetDouble("renderLatency", perf_tracker_.video_paint_ms()); I think ideally we want to have max values for all other *Latency values as well. https://codereview.chromium.org/1811833002/diff/1/remoting/protocol/performan... File remoting/protocol/performance_tracker.cc (right): https://codereview.chromium.org/1811833002/diff/1/remoting/protocol/performan... remoting/protocol/performance_tracker.cc:258: max_round_trip_ms_.Record(round_trip_latency.InMilliseconds()); If you use the same class for Average() and Max() then the same value will not need to be recorded in two places, like this. https://codereview.chromium.org/1811833002/diff/1/remoting/webapp/base/js/con... File remoting/webapp/base/js/connection_stats.js (right): https://codereview.chromium.org/1811833002/diff/1/remoting/webapp/base/js/con... remoting/webapp/base/js/connection_stats.js:130: return value.toFixed(2) + ' ' + units; Please change this to 1 instead of 2. All average values are calculated from 10 integer samples, so the second digit is always 0, so we don't need to display it. Also the Max value will always be integer, so it can be formatted as integer. Maybe pass number of digits after period as a parameter to this function?
https://codereview.chromium.org/1811833002/diff/1/remoting/base/running_max.h File remoting/base/running_max.h (right): https://codereview.chromium.org/1811833002/diff/1/remoting/base/running_max.h... remoting/base/running_max.h:21: class RunningMax { On 2016/03/17 19:12:37, Sergey Ulanov wrote: > I don't think we really need both RunningAverage class and RunningMax. Instead > it would be better to have a single class that host last N samples for the value > and can return both average and max. Just add Max() in RunningAverage, and maybe > rename that class? Adding Max() to RunningAverage without renaming the class may be the safest approach, but the name will be slightly confusing... For renaming the class, I'm slightly concern about the dependency/compatibility issue. Is this something we need to concern about? https://codereview.chromium.org/1811833002/diff/1/remoting/client/plugin/chro... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1811833002/diff/1/remoting/client/plugin/chro... remoting/client/plugin/chromoting_instance.cc:1040: data->SetDouble("renderLatency", perf_tracker_.video_paint_ms()); On 2016/03/17 19:12:37, Sergey Ulanov wrote: > I think ideally we want to have max values for all other *Latency values as > well. so we will have *Latency for average latency and max*Latency for max latency, right? https://codereview.chromium.org/1811833002/diff/1/remoting/protocol/performan... File remoting/protocol/performance_tracker.cc (right): https://codereview.chromium.org/1811833002/diff/1/remoting/protocol/performan... remoting/protocol/performance_tracker.cc:258: max_round_trip_ms_.Record(round_trip_latency.InMilliseconds()); On 2016/03/17 19:12:37, Sergey Ulanov wrote: > If you use the same class for Average() and Max() then the same value will not > need to be recorded in two places, like this. Acknowledged. https://codereview.chromium.org/1811833002/diff/1/remoting/webapp/base/js/con... File remoting/webapp/base/js/connection_stats.js (right): https://codereview.chromium.org/1811833002/diff/1/remoting/webapp/base/js/con... remoting/webapp/base/js/connection_stats.js:130: return value.toFixed(2) + ' ' + units; On 2016/03/17 19:12:37, Sergey Ulanov wrote: > Please change this to 1 instead of 2. All average values are calculated from 10 > integer samples, so the second digit is always 0, so we don't need to display > it. Also the Max value will always be integer, so it can be formatted as > integer. Maybe pass number of digits after period as a parameter to this > function? Acknowledged.
https://codereview.chromium.org/1811833002/diff/1/remoting/base/running_max.h File remoting/base/running_max.h (right): https://codereview.chromium.org/1811833002/diff/1/remoting/base/running_max.h... remoting/base/running_max.h:21: class RunningMax { On 2016/03/17 20:24:44, yuweih wrote: > On 2016/03/17 19:12:37, Sergey Ulanov wrote: > > I don't think we really need both RunningAverage class and RunningMax. Instead > > it would be better to have a single class that host last N samples for the > value > > and can return both average and max. Just add Max() in RunningAverage, and > maybe > > rename that class? > > Adding Max() to RunningAverage without renaming the class may be the safest > approach, but the name will be slightly confusing... > > For renaming the class, I'm slightly concern about the dependency/compatibility > issue. Is this something we need to concern about? Not sure what's the concern about. The class shouldn't be used outside of chromium if that's what you are worried about.
https://codereview.chromium.org/1811833002/diff/1/remoting/client/plugin/chro... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1811833002/diff/1/remoting/client/plugin/chro... remoting/client/plugin/chromoting_instance.cc:1040: data->SetDouble("renderLatency", perf_tracker_.video_paint_ms()); On 2016/03/17 19:12:37, Sergey Ulanov wrote: > I think ideally we want to have max values for all other *Latency values as > well. We can have max values for all latencies but we probably don't have enough space (UI) to show all of them on the status bar...
https://codereview.chromium.org/1811833002/diff/1/remoting/base/running_max.h File remoting/base/running_max.h (right): https://codereview.chromium.org/1811833002/diff/1/remoting/base/running_max.h... remoting/base/running_max.h:21: class RunningMax { On 2016/03/17 22:00:26, Sergey Ulanov wrote: > On 2016/03/17 20:24:44, yuweih wrote: > > On 2016/03/17 19:12:37, Sergey Ulanov wrote: > > > I don't think we really need both RunningAverage class and RunningMax. > Instead > > > it would be better to have a single class that host last N samples for the > > value > > > and can return both average and max. Just add Max() in RunningAverage, and > > maybe > > > rename that class? > > > > Adding Max() to RunningAverage without renaming the class may be the safest > > approach, but the name will be slightly confusing... > > > > For renaming the class, I'm slightly concern about the > dependency/compatibility > > issue. Is this something we need to concern about? > > Not sure what's the concern about. The class shouldn't be used outside of > chromium if that's what you are worried about. Cool... Will rename it to RunningSample
Uploaded version 2. Reused RunningAverage and renamed it to RunningSamples. Other changes are mentioned in the comments. PTAL:) https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... File remoting/base/running_samples_unittest.cc (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... remoting/base/running_samples_unittest.cc:20: static void TestFramework(int windowSize, TestFunction testFn) { Modified from running_average_unittest.cc and added a few tests for Max(). Used lambda to refactor the code. Looks like C++11 lambda is basically supported in chromium code base (http://chromium-cpp.appspot.com/), I can roll it back if it's not the case... https://codereview.chromium.org/1811833002/diff/20001/remoting/client/plugin/... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/client/plugin/... remoting/client/plugin/chromoting_instance.cc:1042: data->SetDouble("maxCaptureLatency", perf_tracker_.max_video_capture_ms()); Looks like there is no SetLong so I just use SetDouble... https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... File remoting/webapp/base/js/connection_stats.js (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... remoting/webapp/base/js/connection_stats.js:144: ' / ' + formatStatNumber(stats.maxCaptureLatency, 'ms', 0) + average / max. Tried to save some space...
https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... File remoting/base/running_samples.cc (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... remoting/base/running_samples.cc:15: sum_(0) { while you are here please remove this initializer and add =0 in the header. https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... File remoting/base/running_samples.h (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... remoting/base/running_samples.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. Please update year for moved or renamed files. Also please remove (c). https://www.chromium.org/developers/coding-style#TOC-File-headers https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... remoting/base/running_samples.h:18: // Calculates the average of the most recent N recorded samples. update this comment https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... remoting/base/running_samples.h:31: double Average(); mark this const. https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... remoting/base/running_samples.h:35: int64_t Max(); const https://codereview.chromium.org/1811833002/diff/20001/remoting/protocol/perfo... File remoting/protocol/performance_tracker.h (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/protocol/perfo... remoting/protocol/performance_tracker.h:59: int64_t max_video_capture_ms() { return video_capture_ms_.Max(); } Update the functions above to return RunningSamples (by const reference). Then you wouldn't need this function. https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... File remoting/webapp/base/js/connection_stats.js (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... remoting/webapp/base/js/connection_stats.js:132: var rounded = (digits == undefined ? digits : (value.toFixed(digits))); Why would digits be undefined here? https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... remoting/webapp/base/js/connection_stats.js:144: ' / ' + formatStatNumber(stats.maxCaptureLatency, 'ms', 0) + On 2016/03/18 01:46:20, yuweih wrote: > average / max. Tried to save some space... This should be clear from the string. Maybe also remove units marker to save more space? I suggest rearranging them to put more important numbers first, e.g. the whole string may looks something like this. (avg, max in ms) RTT: 100.0, 200; Capture: 2.2, 55; Encode: 3.3, 41; Decode: 3.5, 12; Render: 35.4, 55; FPS: 3; Bandwidth: 3kbps (optionally you can use replace this single line with a multi-line transparent block, that would make these stats more readable and scalable)
https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... File remoting/base/running_samples.cc (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... remoting/base/running_samples.cc:15: sum_(0) { On 2016/03/18 19:42:06, Sergey Ulanov wrote: > while you are here please remove this initializer and add =0 in the header. Then how can we construct RunningSamples...? https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... File remoting/base/running_samples.h (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... remoting/base/running_samples.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2016/03/18 19:42:06, Sergey Ulanov wrote: > Please update year for moved or renamed files. Also please remove (c). > https://www.chromium.org/developers/coding-style#TOC-File-headers Acknowledged. https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... remoting/base/running_samples.h:18: // Calculates the average of the most recent N recorded samples. On 2016/03/18 19:42:06, Sergey Ulanov wrote: > update this comment Acknowledged. https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... remoting/base/running_samples.h:31: double Average(); On 2016/03/18 19:42:06, Sergey Ulanov wrote: > mark this const. Acknowledged. https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... remoting/base/running_samples.h:35: int64_t Max(); On 2016/03/18 19:42:06, Sergey Ulanov wrote: > const Acknowledged. https://codereview.chromium.org/1811833002/diff/20001/remoting/protocol/perfo... File remoting/protocol/performance_tracker.h (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/protocol/perfo... remoting/protocol/performance_tracker.h:59: int64_t max_video_capture_ms() { return video_capture_ms_.Max(); } On 2016/03/18 19:42:06, Sergey Ulanov wrote: > Update the functions above to return RunningSamples (by const reference). Then > you wouldn't need this function. Acknowledged. https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... File remoting/webapp/base/js/connection_stats.js (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... remoting/webapp/base/js/connection_stats.js:132: var rounded = (digits == undefined ? digits : (value.toFixed(digits))); On 2016/03/18 19:42:06, Sergey Ulanov wrote: > Why would digits be undefined here? Just thinking it is possible that we may want to format the stat but don't want to make the number fixed decimal. That's simply value+unit, maybe not necessary to be done in a function... (BTW should be value:value.toFixed not digits:value.toFixed...) https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... remoting/webapp/base/js/connection_stats.js:144: ' / ' + formatStatNumber(stats.maxCaptureLatency, 'ms', 0) + On 2016/03/18 19:42:06, Sergey Ulanov wrote: > On 2016/03/18 01:46:20, yuweih wrote: > > average / max. Tried to save some space... > > This should be clear from the string. Maybe also remove units marker to save > more space? > I suggest rearranging them to put more important numbers first, e.g. the whole > string may looks something like this. > (avg, max in ms) RTT: 100.0, 200; Capture: 2.2, 55; Encode: 3.3, 41; Decode: > 3.5, 12; Render: 35.4, 55; FPS: 3; Bandwidth: 3kbps > > (optionally you can use replace this single line with a multi-line transparent > block, that would make these stats more readable and scalable) Do you mean the back-tick string template (`foo: ${foo}`) thing?
https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... File remoting/base/running_samples.cc (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... remoting/base/running_samples.cc:15: sum_(0) { On 2016/03/18 20:05:59, yuweih wrote: > On 2016/03/18 19:42:06, Sergey Ulanov wrote: > > while you are here please remove this initializer and add =0 in the header. > > Then how can we construct RunningSamples...? Sorry for not being clear. My comment was only about sum_(0) initializer, not the constructor. C++11 allows to set default initialization value in the class definition, i.e.: class A { private: int sum_ = 0; }; This will ensure that all constructors that don't have explicit intializer will set sum_ to 0. This code was written before C++11, so it still uses old style initialization. https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... File remoting/webapp/base/js/connection_stats.js (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... remoting/webapp/base/js/connection_stats.js:132: var rounded = (digits == undefined ? digits : (value.toFixed(digits))); On 2016/03/18 20:06:00, yuweih wrote: > On 2016/03/18 19:42:06, Sergey Ulanov wrote: > > Why would digits be undefined here? > > Just thinking it is possible that we may want to format the stat but don't want > to make the number fixed decimal. That's simply value+unit, maybe not necessary > to be done in a function... (BTW should be value:value.toFixed not > digits:value.toFixed...) In that case parameter would need to be marked as optional. But you don't use it below, so you don't need to make it optional now. https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... remoting/webapp/base/js/connection_stats.js:144: ' / ' + formatStatNumber(stats.maxCaptureLatency, 'ms', 0) + On 2016/03/18 20:06:00, yuweih wrote: > On 2016/03/18 19:42:06, Sergey Ulanov wrote: > > On 2016/03/18 01:46:20, yuweih wrote: > > > average / max. Tried to save some space... > > > > This should be clear from the string. Maybe also remove units marker to save > > more space? > > I suggest rearranging them to put more important numbers first, e.g. the whole > > string may looks something like this. > > (avg, max in ms) RTT: 100.0, 200; Capture: 2.2, 55; Encode: 3.3, 41; Decode: > > 3.5, 12; Render: 35.4, 55; FPS: 3; Bandwidth: 3kbps > > > > (optionally you can use replace this single line with a multi-line transparent > > block, that would make these stats more readable and scalable) > > Do you mean the back-tick string template (`foo: ${foo}`) thing? no. I was suggesting to change the UI that display stats.
https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... File remoting/base/running_samples.cc (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/base/running_s... remoting/base/running_samples.cc:15: sum_(0) { On 2016/03/18 20:17:08, Sergey Ulanov wrote: > On 2016/03/18 20:05:59, yuweih wrote: > > On 2016/03/18 19:42:06, Sergey Ulanov wrote: > > > while you are here please remove this initializer and add =0 in the header. > > > > Then how can we construct RunningSamples...? > > Sorry for not being clear. My comment was only about sum_(0) initializer, not > the constructor. C++11 allows to set default initialization value in the class > definition, i.e.: > > class A { > private: > int sum_ = 0; > }; > > This will ensure that all constructors that don't have explicit intializer will > set sum_ to 0. This code was written before C++11, so it still uses old style > initialization. Got it:) https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... File remoting/webapp/base/js/connection_stats.js (right): https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... remoting/webapp/base/js/connection_stats.js:132: var rounded = (digits == undefined ? digits : (value.toFixed(digits))); On 2016/03/18 20:17:08, Sergey Ulanov wrote: > On 2016/03/18 20:06:00, yuweih wrote: > > On 2016/03/18 19:42:06, Sergey Ulanov wrote: > > > Why would digits be undefined here? > > > > Just thinking it is possible that we may want to format the stat but don't > want > > to make the number fixed decimal. That's simply value+unit, maybe not > necessary > > to be done in a function... (BTW should be value:value.toFixed not > > digits:value.toFixed...) > > In that case parameter would need to be marked as optional. But you don't use it > below, so you don't need to make it optional now. Acknowledged. https://codereview.chromium.org/1811833002/diff/20001/remoting/webapp/base/js... remoting/webapp/base/js/connection_stats.js:144: ' / ' + formatStatNumber(stats.maxCaptureLatency, 'ms', 0) + On 2016/03/18 20:17:08, Sergey Ulanov wrote: > On 2016/03/18 20:06:00, yuweih wrote: > > On 2016/03/18 19:42:06, Sergey Ulanov wrote: > > > On 2016/03/18 01:46:20, yuweih wrote: > > > > average / max. Tried to save some space... > > > > > > This should be clear from the string. Maybe also remove units marker to save > > > more space? > > > I suggest rearranging them to put more important numbers first, e.g. the > whole > > > string may looks something like this. > > > (avg, max in ms) RTT: 100.0, 200; Capture: 2.2, 55; Encode: 3.3, 41; Decode: > > > 3.5, 12; Render: 35.4, 55; FPS: 3; Bandwidth: 3kbps > > > > > > (optionally you can use replace this single line with a multi-line > transparent > > > block, that would make these stats more readable and scalable) > > > > Do you mean the back-tick string template (`foo: ${foo}`) thing? > > no. I was suggesting to change the UI that display stats. I will change the UI to display stats with less space. I'm not sure what you mean by multi-line transparent block if you are not talking about the back-tick thing... Maybe I can simply make a function that generate `NAME: AVERAGE, MAX;` string to refactor the code a little bit...
Version 3 is uploaded. * Reviewer's feedback for const RunningSamples * Status bar UI changed to save more space
yuweih@chromium.org changed reviewers: + kelvinp@chromium.org
Version 4 contains code for client to stats to the telemetry server. Since this code is basically dependent on code for gathering these stats, I just put the code into the same CL. Kelvin, would you take a look at these files: * remoting/webapp/base/js/session_logger.js * remoting/webapp/base/js/chromoting_event.js * remoting/webapp/base/js/stats_accumulator.js Thanks!
lgtm with nits https://codereview.chromium.org/1811833002/diff/60001/remoting/webapp/base/js... File remoting/webapp/base/js/stats_accumulator.js (right): https://codereview.chromium.org/1811833002/diff/60001/remoting/webapp/base/js... remoting/webapp/base/js/stats_accumulator.js:108: if (values.length == 0) { can be simplified as var calcMax = function(values) { return Math.max.apply(null, values); }
On 2016/03/18 20:46:47, yuweih wrote: > I will change the UI to display stats with less space. I'm not sure > what you mean by multi-line transparent block if you are not talking about the > back-tick thing... Maybe I can simply make a function that generate `NAME: > AVERAGE, MAX;` string to refactor the code a little bit... Currently the stats bar is a single line in the bottom of the display. Potentially, to make it more readable it can be changed to show the stats in several lines of text.
On 2016/03/21 19:21:10, Sergey Ulanov wrote: > On 2016/03/18 20:46:47, yuweih wrote: > > I will change the UI to display stats with less space. I'm not sure > > what you mean by multi-line transparent block if you are not talking about the > > back-tick thing... Maybe I can simply make a function that generate `NAME: > > AVERAGE, MAX;` string to refactor the code a little bit... > > Currently the stats bar is a single line in the bottom of the display. > Potentially, to make it more readable it can be changed to show the stats in > several lines of text. Got it, but the (avg, max) format saves much space and looks like single line is good enough for now. Maybe we can consider changing this to multi-lines later?
https://codereview.chromium.org/1811833002/diff/60001/remoting/webapp/base/js... File remoting/webapp/base/js/stats_accumulator.js (right): https://codereview.chromium.org/1811833002/diff/60001/remoting/webapp/base/js... remoting/webapp/base/js/stats_accumulator.js:108: if (values.length == 0) { On 2016/03/21 19:14:36, kelvinp wrote: > can be simplified as > > var calcMax = function(values) { > return Math.max.apply(null, values); > } Acknowledged. Interesting that Math.max doesn't have a max(numberArray) form...
lgtm after my comments are addressed. https://codereview.chromium.org/1811833002/diff/60001/remoting/base/running_s... File remoting/base/running_samples.cc (right): https://codereview.chromium.org/1811833002/diff/60001/remoting/base/running_s... remoting/base/running_samples.cc:46: // O(n*w), may be improved. What is w? I don't think you really need this comment. It's obvious it's linear complexity, but in this case it's not worthwhile to try improving it. https://codereview.chromium.org/1811833002/diff/60001/remoting/client/plugin/... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1811833002/diff/60001/remoting/client/plugin/... remoting/client/plugin/chromoting_instance.cc:1042: data->SetDouble("maxCaptureLatency", perf_tracker_.video_capture_ms().Max()); maybe reorder these so max is always after average? https://codereview.chromium.org/1811833002/diff/60001/remoting/webapp/base/js... File remoting/webapp/base/js/stats_accumulator.js (right): https://codereview.chromium.org/1811833002/diff/60001/remoting/webapp/base/js... remoting/webapp/base/js/stats_accumulator.js:122: // Can refactor calcMean and calcMax in functional way If there is a good reason to refactor them then this should be a TODO. If there is no reason to refactor them, then you don't need this comment.
https://codereview.chromium.org/1811833002/diff/60001/remoting/base/running_s... File remoting/base/running_samples.cc (right): https://codereview.chromium.org/1811833002/diff/60001/remoting/base/running_s... remoting/base/running_samples.cc:46: // O(n*w), may be improved. On 2016/03/21 19:35:06, Sergey Ulanov wrote: > What is w? I don't think you really need this comment. It's obvious it's linear > complexity, but in this case it's not worthwhile to try improving it. w just means window size so finding max after having n samples will be O(n*w)... This is just the possibility of micro-optimization, so I will remove this comment... https://codereview.chromium.org/1811833002/diff/60001/remoting/client/plugin/... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1811833002/diff/60001/remoting/client/plugin/... remoting/client/plugin/chromoting_instance.cc:1042: data->SetDouble("maxCaptureLatency", perf_tracker_.video_capture_ms().Max()); On 2016/03/21 19:35:06, Sergey Ulanov wrote: > maybe reorder these so max is always after average? Sure. Then duplicating lines will be easier https://codereview.chromium.org/1811833002/diff/60001/remoting/webapp/base/js... File remoting/webapp/base/js/stats_accumulator.js (right): https://codereview.chromium.org/1811833002/diff/60001/remoting/webapp/base/js... remoting/webapp/base/js/stats_accumulator.js:122: // Can refactor calcMean and calcMax in functional way On 2016/03/21 19:35:06, Sergey Ulanov wrote: > If there is a good reason to refactor them then this should be a TODO. If there > is no reason to refactor them, then you don't need this comment. Acknowledged.
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, kelvinp@chromium.org Link to the patchset: https://codereview.chromium.org/1811833002/#ps80001 (title: "[Remoting WebApp]Show and upload max capture/encode/render/roundtrip latency")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811833002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811833002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, kelvinp@chromium.org Link to the patchset: https://codereview.chromium.org/1811833002/#ps100001 (title: "[Remoting WebApp]Show and upload max capture/encode/render/roundtrip latency")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811833002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811833002/100001
https://codereview.chromium.org/1811833002/diff/60001/remoting/base/running_s... File remoting/base/running_samples.cc (right): https://codereview.chromium.org/1811833002/diff/60001/remoting/base/running_s... remoting/base/running_samples.cc:46: // O(n*w), may be improved. On 2016/03/21 19:59:40, yuweih wrote: > On 2016/03/21 19:35:06, Sergey Ulanov wrote: > > What is w? I don't think you really need this comment. It's obvious it's > linear > > complexity, but in this case it's not worthwhile to try improving it. > > w just means window size so finding max after having n samples will be O(n*w)... > This is just the possibility of micro-optimization, so I will remove this > comment... Not sure why it's O(n*w) and not O(w). There are less than w elements in data_points_ and you iterate over them to find the max. I don't see how n play a part here.
On 2016/03/21 23:54:52, Sergey Ulanov wrote: > https://codereview.chromium.org/1811833002/diff/60001/remoting/base/running_s... > File remoting/base/running_samples.cc (right): > > https://codereview.chromium.org/1811833002/diff/60001/remoting/base/running_s... > remoting/base/running_samples.cc:46: // O(n*w), may be improved. > On 2016/03/21 19:59:40, yuweih wrote: > > On 2016/03/21 19:35:06, Sergey Ulanov wrote: > > > What is w? I don't think you really need this comment. It's obvious it's > > linear > > > complexity, but in this case it's not worthwhile to try improving it. > > > > w just means window size so finding max after having n samples will be > O(n*w)... > > This is just the possibility of micro-optimization, so I will remove this > > comment... > > Not sure why it's O(n*w) and not O(w). There are less than w elements in > data_points_ and you iterate over them to find the max. I don't see how n play a > part here. For one iteration it is O(w), so I think it is more accurate to write O(w) instead of O(n*w)... What I meant was the time complexity of running all n iterations... BTW the refactorization of RunningSamples broke the code for Android and iOS and I just fixed in patch 6...
https://codereview.chromium.org/1811833002/diff/100001/remoting/client/jni/ch... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/1811833002/diff/100001/remoting/client/jni/ch... remoting/client/jni/chromoting_jni_instance.cc:501: ); nit: move this to the previous line, or run clang-format to move it for you: https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md
The CQ bit was unchecked by yuweih@chromium.org
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, kelvinp@chromium.org Link to the patchset: https://codereview.chromium.org/1811833002/#ps120001 (title: "[Remoting WebApp]Show and upload max capture/encode/render/roundtrip latency")
The CQ bit was unchecked by yuweih@chromium.org
The CQ bit was checked by yuweih@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811833002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811833002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, kelvinp@chromium.org Link to the patchset: https://codereview.chromium.org/1811833002/#ps140001 (title: "[Remoting WebApp]Show and upload max capture/encode/render/roundtrip latency")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811833002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811833002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by yuweih@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811833002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811833002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Show max latency on client's status bar Will later also consider storing max latency data in server BUG=560950 ========== to ========== Show max latency on client's status bar Will later also consider storing max latency data in server BUG=560950 Committed: https://crrev.com/e37a467453f3814a9ecb0eb6769bd7c58b61cbae Cr-Commit-Position: refs/heads/master@{#382663} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e37a467453f3814a9ecb0eb6769bd7c58b61cbae Cr-Commit-Position: refs/heads/master@{#382663}
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1811833002/diff/140001/remoting/webapp/base/j... File remoting/webapp/base/js/connection_stats.js (right): https://codereview.chromium.org/1811833002/diff/140001/remoting/webapp/base/j... remoting/webapp/base/js/connection_stats.js:126: * @param {number} number of digits after decimal. @param {number} digits
Message was sent while issue was closed.
Patchset #9 (id:160001) has been deleted
Message was sent while issue was closed.
newt@chromium.org changed reviewers: + newt@chromium.org
Message was sent while issue was closed.
This CL caused a compile failure on 64-bit Android bots. https://bugs.chromium.org/p/chromium/issues/detail?id=597155
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1817093005/ by newt@chromium.org. The reason for reverting is: Caused compile failure on 64-bit Android bots. See crbug.com/597155.
Message was sent while issue was closed.
Description was changed from ========== Show max latency on client's status bar Will later also consider storing max latency data in server BUG=560950 Committed: https://crrev.com/e37a467453f3814a9ecb0eb6769bd7c58b61cbae Cr-Commit-Position: refs/heads/master@{#382663} ========== to ========== Show max latency on client's status bar Will later also consider storing max latency data in server BUG=560950 Committed: https://crrev.com/e37a467453f3814a9ecb0eb6769bd7c58b61cbae Cr-Commit-Position: refs/heads/master@{#382663} ==========
Fixed the %lld issue. PTAL
The CQ bit was checked by yuweih@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811833002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811833002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PRId64 instead of lld lgtm. It looks like it'll compile, but note that the green 64-bit android bots didn't actually compile the code, so you might want to double check that it compiles locally.
On 2016/03/23 21:42:12, newt wrote: > PRId64 instead of lld lgtm. It looks like it'll compile, but note that the green > 64-bit android bots didn't actually compile the code, so you might want to > double check that it compiles locally. Verified with target_cpu="arm64"
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, kelvinp@chromium.org Link to the patchset: https://codereview.chromium.org/1811833002/#ps180001 (title: "JS compiler fix and lld fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1811833002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1811833002/180001
Message was sent while issue was closed.
Description was changed from ========== Show max latency on client's status bar Will later also consider storing max latency data in server BUG=560950 Committed: https://crrev.com/e37a467453f3814a9ecb0eb6769bd7c58b61cbae Cr-Commit-Position: refs/heads/master@{#382663} ========== to ========== Show max latency on client's status bar Will later also consider storing max latency data in server BUG=560950 Committed: https://crrev.com/e37a467453f3814a9ecb0eb6769bd7c58b61cbae Cr-Commit-Position: refs/heads/master@{#382663} ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Show max latency on client's status bar Will later also consider storing max latency data in server BUG=560950 Committed: https://crrev.com/e37a467453f3814a9ecb0eb6769bd7c58b61cbae Cr-Commit-Position: refs/heads/master@{#382663} ========== to ========== Show max latency on client's status bar Will later also consider storing max latency data in server BUG=560950 Committed: https://crrev.com/e37a467453f3814a9ecb0eb6769bd7c58b61cbae Cr-Commit-Position: refs/heads/master@{#382663} Committed: https://crrev.com/d2a0c5cb1217570e7b3da52415f7326f48928053 Cr-Commit-Position: refs/heads/master@{#382957} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d2a0c5cb1217570e7b3da52415f7326f48928053 Cr-Commit-Position: refs/heads/master@{#382957} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
