Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10836202:
Refactor/rewrite scroll.js, reporting results as renderingStats objects. (Closed)
|
Created:
8 years, 4 months ago by dtu Modified:
8 years, 4 months ago CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org, Nirnimesh, jam, apatrick_chromium, joi+watch-content_chromium.org, Aaron Boodman, anantha, darin-cc_chromium.org, dyu1, pam+watch_chromium.org, tonyg Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor/rewrite scroll.js.
The file has been rewritten to conform to Google's JavaScript style, moving all of the __private_variables into a class.
Results are now reported as a list of renderingStats objects. If gpuBenchmarking is unavailable, it falls back to measuring frame times using RAF and populating some of the renderingStats fields.
The way the test is called has changed. Previously, you would call __scroll_test(), wait for __scrolling_complete, then read __frame_times. Now you call new ScrollTest(callback), which takes a callback as a parameter, and calls that with the results list when the test is finished. This simplifies the way it is called in perf.py.
gpu_benchmarking_extension.cc no longer checks if the fields are nonzero. This was a problem when the droppedFrameCount really was 0, but it wouldn't populate that field as it should.
BUG=137789, 141477
TEST=This is a test.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152041
Patch Set 1 : Initial commit. #
Total comments: 28
Patch Set 2 : Put RafScrollStats into its own RAF loop. Assorted other cleanup. #
Total comments: 12
Patch Set 3 : More changes. #
Messages
Total messages: 18 (0 generated)
PTAL
C++ stuff LGTM
With these new changes (which are great) you run the risk of namespace
collisions.
Wrap this into a (function() {
})(document, window);
Then, explicitly export any functions that are used to window.__scrollTest or
something similar.
Some more cleanup ideas https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... File chrome/test/data/scroll/scroll.js (right): https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:16: window.performance = window.performance || {}; Its probably bad that we're mutating the window object. We should probably be defining a local variable instead. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:26: window.requestAnimationFrame = (function(){ same comment about mutating https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:39: function BenchmarkingScrollStats() { GpuBenchmarkingScrollStats https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:43: BenchmarkingScrollStats.prototype.getRenderingStats = function() { doesn't seem like it needs to be a member function... https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:45: stats.totalTimeInSeconds = window.performance.now() / 1000; timestampWhenObtained? https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:63: this.results.push(this.getStatsDiff(this.initialStats)); do we need to support more than one results? https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:70: function RAFScrollStats() { this class should set up its own raf loop that records frame times. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:119: this.scrollStats = new BenchmarkingScrollStats(); seems like this should be a private var, and just have a getter on this class that plumbs to this.scrollStats_.results https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:121: this.scrollStats = new RAFScrollStats; any reason for not ()? https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:123: if (__isGmailTest) { feels like this could be a construction time variable? https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:159: this.scrollStats.endStep(timestamp); remove this https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:171: else if (window.domAutomationController) Does anything still use this send case? I wonder if we can modify the python code to hook in toto the scroll test instead? https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:178: this.endStep(timestamp); seems like we should remove this. RAF loop that drives the stats should be independent of the raf loop that drives scrolling. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/functio... chrome/test/functional/perf.py:2206: '--enable-stats-table', any reason this is added here?
Sorry, the anonymous function wrapper completely destroyed the diff. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... File chrome/test/data/scroll/scroll.js (right): https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:16: window.performance = window.performance || {}; On 2012/08/14 00:24:34, nduca wrote: > Its probably bad that we're mutating the window object. We should probably be > defining a local variable instead. Done. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:26: window.requestAnimationFrame = (function(){ On 2012/08/14 00:24:34, nduca wrote: > same comment about mutating Done. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:39: function BenchmarkingScrollStats() { On 2012/08/14 00:24:34, nduca wrote: > GpuBenchmarkingScrollStats Done. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:43: BenchmarkingScrollStats.prototype.getRenderingStats = function() { On 2012/08/14 00:24:34, nduca wrote: > doesn't seem like it needs to be a member function... Is it better in JavaScript to put it outside? RafScrollStats.getDroppedFrameCount() also doesn't need to be a member, but it doesn't really do anything useful outside of the class. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:45: stats.totalTimeInSeconds = window.performance.now() / 1000; On 2012/08/14 00:24:34, nduca wrote: > timestampWhenObtained? As is, it is more consistent with totalPaintTimeInSeconds, etc; and the name makes more sense after it's been diffed. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:63: this.results.push(this.getStatsDiff(this.initialStats)); On 2012/08/14 00:24:34, nduca wrote: > do we need to support more than one results? Done. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:70: function RAFScrollStats() { On 2012/08/14 00:24:34, nduca wrote: > this class should set up its own raf loop that records frame times. Done. Huh, hadn't even occurred to me. That makes the interface cleaner. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:119: this.scrollStats = new BenchmarkingScrollStats(); On 2012/08/14 00:24:34, nduca wrote: > seems like this should be a private var, and just have a getter on this class > that plumbs to this.scrollStats_.results Not sure what you're saying. A getter is not needed because the class returns the result in a callback. The var is not used outside this class, so it could just be named with an underscore. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:121: this.scrollStats = new RAFScrollStats; On 2012/08/14 00:24:34, nduca wrote: > any reason for not ()? Done. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:123: if (__isGmailTest) { On 2012/08/14 00:24:34, nduca wrote: > feels like this could be a construction time variable? Done. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:159: this.scrollStats.endStep(timestamp); On 2012/08/14 00:24:34, nduca wrote: > remove this Done. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:171: else if (window.domAutomationController) On 2012/08/14 00:24:34, nduca wrote: > Does anything still use this send case? I wonder if we can modify the python > code to hook in toto the scroll test instead? Done. On the Python side that looks like: __ScrollTest(function(results) { window.domAutomationController.send(JSON.stringify(results)); }); https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/data/sc... chrome/test/data/scroll/scroll.js:178: this.endStep(timestamp); On 2012/08/14 00:24:34, nduca wrote: > seems like we should remove this. RAF loop that drives the stats should be > independent of the raf loop that drives scrolling. Done. https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/functio... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10836202/diff/5001/chrome/test/functio... chrome/test/functional/perf.py:2206: '--enable-stats-table', On 2012/08/14 00:24:34, nduca wrote: > any reason this is added here? Chrome DCHECKS for --enable-stats-table if --enable-benchmarking is specified. I guess nobody ran this with a Debug build. http://crbug.com/130795
LGTM with nit https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/data/s... File chrome/test/data/scroll/scroll.js (right): https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/data/s... chrome/test/data/scroll/scroll.js:150: ScrollTest.prototype.sendResults_ = function() { you can probably just inline this once you clean up the processStep_ function as noted below https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/data/s... chrome/test/data/scroll/scroll.js:166: if (isScrollComplete) { you can restructure this to be if (!scrollcomplete) startstep return then do the logic for (!testcomplete) startscroll again then finally do the send results logic https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/data/s... chrome/test/data/scroll/scroll.js:181: window.__scrollTest = function(callback, opt_isGmailTest) { How about exposing the constructor directly as window.__ScrollTest? The embedder of this code can store it somewhere.
2 comments for perf.py https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/functi... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/functi... chrome/test/functional/perf.py:589: logging.info('Iteration %d of %d: %f milliseconds', iteration + 1, What is the reason for adding the "+1" to each of these logging messages? I believe the original code already outputs (by default) Iteration 1 of 10 Iteration 2 of 10 ... Iteration 10 of 10 This is because when 10 iterations are needed, it actually does 11 iterations and ignores the first one. With the change in this line, I think it would output: Iteration 2 of 10 Iteration 3 of 10 ... Iteration 11 of 10 https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/functi... chrome/test/functional/perf.py:2209: '--enable-stats-table', This was removed here: https://chromiumcodereview.appspot.com/10829259/diff/1/chrome/test/functional... What do we get from adding it back in?
https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/data/s... File chrome/test/data/scroll/scroll.js (right): https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/data/s... chrome/test/data/scroll/scroll.js:150: ScrollTest.prototype.sendResults_ = function() { On 2012/08/16 02:42:59, nduca wrote: > you can probably just inline this once you clean up the processStep_ function as > noted below Done. https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/data/s... chrome/test/data/scroll/scroll.js:166: if (isScrollComplete) { On 2012/08/16 02:42:59, nduca wrote: > you can restructure this to be if (!scrollcomplete) > startstep > return > > then do the logic for (!testcomplete) > startscroll again > > then finally do the send results logic Done. https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/data/s... chrome/test/data/scroll/scroll.js:181: window.__scrollTest = function(callback, opt_isGmailTest) { On 2012/08/16 02:42:59, nduca wrote: > How about exposing the constructor directly as window.__ScrollTest? The embedder > of this code can store it somewhere. Done. https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/functi... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/functi... chrome/test/functional/perf.py:589: logging.info('Iteration %d of %d: %f milliseconds', iteration + 1, On 2012/08/16 17:50:59, dennis_jeffrey wrote: > What is the reason for adding the "+1" to each of these logging messages? I > believe the original code already outputs (by default) > > Iteration 1 of 10 > Iteration 2 of 10 > ... > Iteration 10 of 10 > > This is because when 10 iterations are needed, it actually does 11 iterations > and ignores the first one. > > With the change in this line, I think it would output: > > Iteration 2 of 10 > Iteration 3 of 10 > ... > Iteration 11 of 10 Done. Sorry, you're right. The only loop that was missing the " + 1" was the one in test2012Q3, since it doesn't skip the first iteration. https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/functi... chrome/test/functional/perf.py:2209: '--enable-stats-table', On 2012/08/16 17:50:59, dennis_jeffrey wrote: > This was removed here: > > https://chromiumcodereview.appspot.com/10829259/diff/1/chrome/test/functional... > > What do we get from adding it back in? Oh, interesting. The reason is, when you have --enable-benchmarking, Chrome DCHECKs for --enable-stats-table, so it'll crash with a Debug build: http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chrome_cont... Do these tests even use --enable-benchmarking? A comment on the bug from your linked CL says it's not used: http://code.google.com/p/chromium/issues/detail?id=133069#c16 In that case they can both be removed.
https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/functi... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/functi... chrome/test/functional/perf.py:2209: '--enable-stats-table', On 2012/08/16 19:12:39, Dave Tu wrote: > On 2012/08/16 17:50:59, dennis_jeffrey wrote: > > This was removed here: > > > > > https://chromiumcodereview.appspot.com/10829259/diff/1/chrome/test/functional... > > > > What do we get from adding it back in? > > Oh, interesting. > > The reason is, when you have --enable-benchmarking, Chrome DCHECKs for > --enable-stats-table, so it'll crash with a Debug build: > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chrome_cont... > > Do these tests even use --enable-benchmarking? A comment on the bug from your > linked CL says it's not used: > http://code.google.com/p/chromium/issues/detail?id=133069#c16 > > In that case they can both be removed. I was mistaken about --enable-benchmarking not being used. The test does not use stats from it, however, the API is used to clear browser data.
https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/functi... File chrome/test/functional/perf.py (right): https://chromiumcodereview.appspot.com/10836202/diff/10006/chrome/test/functi... chrome/test/functional/perf.py:2209: '--enable-stats-table', On 2012/08/16 19:33:52, slamm wrote: > On 2012/08/16 19:12:39, Dave Tu wrote: > > On 2012/08/16 17:50:59, dennis_jeffrey wrote: > > > This was removed here: > > > > > > > > > https://chromiumcodereview.appspot.com/10829259/diff/1/chrome/test/functional... > > > > > > What do we get from adding it back in? > > > > Oh, interesting. > > > > The reason is, when you have --enable-benchmarking, Chrome DCHECKs for > > --enable-stats-table, so it'll crash with a Debug build: > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chrome_cont... > > > > Do these tests even use --enable-benchmarking? A comment on the bug from your > > linked CL says it's not used: > > http://code.google.com/p/chromium/issues/detail?id=133069#c16 > > > > In that case they can both be removed. > > I was mistaken about --enable-benchmarking not being used. > The test does not use stats from it, however, the API is used to clear browser > data. So if we remove --enable-stats-table, then we'll hit a DCHECK on debug builds. But if we add it back in, then the issue mentioned here would likely crop up again: http://code.google.com/p/chromium/issues/detail?id=133069#c19 Not sure what the best solution is here, but some options mentioned by slamm@: (1) do not add the flag, and turn this test off for debug builds, since anyway the test would run a lot more slowly on debug builds (2) add this flag, but only when running with a debug build (3) see if --enable-stats-table is really needed by --enable-benchmarking, and doing some cleanup around that if possible.
I think we should land as is and do a follow up bug. This particular issue is lower priority than getting this test running.
On 2012/08/16 21:02:00, nduca wrote: > I think we should land as is and do a follow up bug. This particular issue is > lower priority than getting this test running. I agree with solving this in a follow-up bug. In the meantime, if adding the --enable-stats-table flag is not necessary to get your tests running (I assume you wouldn't be actively running the test with a debug build anyway?), I'd prefer we *not* add the flag for now so that we don't break slamm@'s test in the meantime.
On 2012/08/16 21:08:09, dennis_jeffrey wrote: > On 2012/08/16 21:02:00, nduca wrote: > > I think we should land as is and do a follow up bug. This particular issue is > > lower priority than getting this test running. > > I agree with solving this in a follow-up bug. In the meantime, if adding the > --enable-stats-table flag is not necessary to get your tests running (I assume > you wouldn't be actively running the test with a debug build anyway?), I'd > prefer we *not* add the flag for now so that we don't break slamm@'s test in the > meantime. Sounds good. PTAL
LGTM thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtu@chromium.org/10836202/11006
Try job failure for 10836202-11006 (retry) on linux_rel for step "interactive_ui_tests". It's a second try, previously, step "interactive_ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtu@chromium.org/10836202/11006
Change committed as 152041 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
