Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10837223:
Tiled rendering microbenchmarks (Closed)
|
Created:
8 years, 4 months ago by dmurph Modified:
8 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, scheib Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionTiled rendering microbenchmarks
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151783
Patch Set 1 #Patch Set 2 : fixed lint errors #
Total comments: 35
Patch Set 3 : simplifications + fixes #Patch Set 4 : lint fixes #Patch Set 5 : small cleanup #
Total comments: 4
Patch Set 6 : Patch #
Total comments: 2
Patch Set 7 : highres time #Messages
Total messages: 14 (0 generated)
Added tiled microbenchmarks, with both square and layer-width tiles. Also added canvas count metric to the null canvas benchmark.
https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... File content/renderer/all_rendering_benchmarks.cc (right): https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:13: #include "base/memory/scoped_ptr.h" nit: alpha order please. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:32: // Base class for timing the painting to custom WebCanvases nit: comments end with a . https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:97: WebViewBenchmarkSupport* support) OVERRIDE { nit: indentation https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:135: // Base class for timing replaying nit: . https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:175: const WebSize& CurrLayerSize() { nit: you can name this current_layer_size() since it's a trivial accessor. Also, make the method const. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:184: WebSize curr_layer_size_; nit: s/curr/current/ We tend to avoid abbreviations. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:206: TimeDelta timeAccum; nit: As ment'n abv, we don't <3 abbrevs. Also, naming style. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:209: scoped_ptr<WebCanvas> canvas(canvas_provider.createCanvas(canvas_size)); It would be useful I think to clip the last column and row so that we don't paint more than the actual size of the SkPicture. That way you don't skew the results depending on the tile size. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:211: TimeTicks before_time = TimeTicks::Now(); I'm not convinced that this benchmark accurately represents the compositor pattern if you don't count the time to create the canvas, which, unfortunately, is not 0. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:239: TimeDelta timeAccum; nit: style, abbrev. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:240: for (int y = 0; y < picture->height(); y += tile_height_) { same comments as in SquareTiledReplayBenchmark, you probably want to clip the last row, and you may want to count the createCanvas time. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:257: SkDevice* device = new SkDevice(SkBitmap::kARGB_8888_Config, I think there's a SkTAutoUnref or something that can do the unref for you.
https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... File content/renderer/all_rendering_benchmarks.cc (right): https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:32: // Base class for timing the painting to custom WebCanvases complete sentences. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:102: "i", Lets merge these three strings into one. "resultName". This flexibility is unneeded and showing its awkwardness here. I think this should be a standalone benchmark. NumCanvasesCreated is result name. You can make that change after this basic change lands, though. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:124: picture_.endRecording(); What if endRecording has costs? Make sure that the cost of that gets included in the timings. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:141: public: Do this with a pure method on the TiledREplayBenchmark api and then have the subclasses implement directly. No new abstraction needed. Thats overdesign. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:155: return picture_.beginRecording(size.width, size.height); er, you're recording all the different canvases into the same picture, and alternating between recording and painting. Dont do that. Make a different picture for every canvas. Record everything. Then paint everything. Dont measure the record time. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:202: virtual TimeDelta timeTiledRepaint( why not factor this into a "get me a vector of rects to paint." Thats what the subclass would do. The parent class then does everything else.
https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... File content/renderer/all_rendering_benchmarks.cc (right): https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:13: #include "base/memory/scoped_ptr.h" On 2012/08/13 18:54:52, piman wrote: > nit: alpha order please. Done. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:32: // Base class for timing the painting to custom WebCanvases On 2012/08/13 19:05:03, nduca wrote: > complete sentences. Done. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:97: WebViewBenchmarkSupport* support) OVERRIDE { On 2012/08/13 18:54:52, piman wrote: > nit: indentation Done. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:102: "i", On 2012/08/13 19:05:03, nduca wrote: > Lets merge these three strings into one. "resultName". This flexibility is > unneeded and showing its awkwardness here. > > I think this should be a standalone benchmark. NumCanvasesCreated is result > name. > > You can make that change after this basic change lands, though. Ok, I'll do that in the next cl. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:124: picture_.endRecording(); On 2012/08/13 19:05:03, nduca wrote: > What if endRecording has costs? Make sure that the cost of that gets included in > the timings. Done. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:135: // Base class for timing replaying On 2012/08/13 18:54:52, piman wrote: > nit: . Done. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:141: public: On 2012/08/13 19:05:03, nduca wrote: > Do this with a pure method on the TiledREplayBenchmark api and then have the > subclasses implement directly. No new abstraction needed. Thats overdesign. I was under the impression that we would be using different canvases to replay onto (Bitmap or SkGpuDevice). I can simplify everything by assuming only bitmap, and if you guys need multiple canvas types then you can re-add the abstraction. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:155: return picture_.beginRecording(size.width, size.height); On 2012/08/13 19:05:03, nduca wrote: > er, you're recording all the different canvases into the same picture, and > alternating between recording and painting. Dont do that. Make a different > picture for every canvas. Record everything. Then paint everything. Dont measure > the record time. Not quite, I only time stuff on didPaint(), I'm not measuring any recording here. And according to Alok's work for serialization, I can reuse the same SkPicture. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:175: const WebSize& CurrLayerSize() { On 2012/08/13 18:54:52, piman wrote: > nit: you can name this current_layer_size() since it's a trivial accessor. Also, > make the method const. Ended up being redundant, removed. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:184: WebSize curr_layer_size_; On 2012/08/13 18:54:52, piman wrote: > nit: s/curr/current/ > > We tend to avoid abbreviations. Ended up being redundant, removed. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:202: virtual TimeDelta timeTiledRepaint( On 2012/08/13 19:05:03, nduca wrote: > why not factor this into a "get me a vector of rects to paint." Thats what the > subclass would do. The parent class then does everything else. Yeah, that's much better. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:206: TimeDelta timeAccum; On 2012/08/13 18:54:52, piman wrote: > nit: As ment'n abv, we don't <3 abbrevs. > Also, naming style. Done. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:209: scoped_ptr<WebCanvas> canvas(canvas_provider.createCanvas(canvas_size)); On 2012/08/13 18:54:52, piman wrote: > It would be useful I think to clip the last column and row so that we don't > paint more than the actual size of the SkPicture. That way you don't skew the > results depending on the tile size. Done. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:211: TimeTicks before_time = TimeTicks::Now(); On 2012/08/13 18:54:52, piman wrote: > I'm not convinced that this benchmark accurately represents the compositor > pattern if you don't count the time to create the canvas, which, unfortunately, > is not 0. You're right, fixed. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:239: TimeDelta timeAccum; On 2012/08/13 18:54:52, piman wrote: > nit: style, abbrev. Done. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:240: for (int y = 0; y < picture->height(); y += tile_height_) { On 2012/08/13 18:54:52, piman wrote: > same comments as in SquareTiledReplayBenchmark, you probably want to clip the > last row, and you may want to count the createCanvas time. Done. https://chromiumcodereview.appspot.com/10837223/diff/3/content/renderer/all_r... content/renderer/all_rendering_benchmarks.cc:257: SkDevice* device = new SkDevice(SkBitmap::kARGB_8888_Config, On 2012/08/13 18:54:52, piman wrote: > I think there's a SkTAutoUnref or something that can do the unref for you. Done.
small change to remove the custom bitmap canvas and use the one that the compositor uses.
Just a couple of things left from my side. https://chromiumcodereview.appspot.com/10837223/diff/10001/content/renderer/a... File content/renderer/all_rendering_benchmarks.cc (right): https://chromiumcodereview.appspot.com/10837223/diff/10001/content/renderer/a... content/renderer/all_rendering_benchmarks.cc:162: paint_time_total_ += (TimeTicks::Now() - before_time); well, it sounds like your timing scope could simply be outside of the for loop now. https://chromiumcodereview.appspot.com/10837223/diff/10001/content/renderer/a... content/renderer/all_rendering_benchmarks.cc:177: virtual vector<WebRect> getRepaintTiles(const WebSize& layer_size) const = 0; nit: GetRepaintTiles
https://chromiumcodereview.appspot.com/10837223/diff/10001/content/renderer/a... File content/renderer/all_rendering_benchmarks.cc (right): https://chromiumcodereview.appspot.com/10837223/diff/10001/content/renderer/a... content/renderer/all_rendering_benchmarks.cc:162: paint_time_total_ += (TimeTicks::Now() - before_time); On 2012/08/14 03:26:05, piman wrote: > well, it sounds like your timing scope could simply be outside of the for loop > now. Done. https://chromiumcodereview.appspot.com/10837223/diff/10001/content/renderer/a... content/renderer/all_rendering_benchmarks.cc:177: virtual vector<WebRect> getRepaintTiles(const WebSize& layer_size) const = 0; On 2012/08/14 03:26:05, piman wrote: > nit: GetRepaintTiles Done.
LGTM, I'm happy, but also check with Nat.
LGTM but please resolve concerns below https://chromiumcodereview.appspot.com/10837223/diff/7004/content/renderer/al... File content/renderer/all_rendering_benchmarks.cc (right): https://chromiumcodereview.appspot.com/10837223/diff/7004/content/renderer/al... content/renderer/all_rendering_benchmarks.cc:158: scoped_ptr<WebCanvas> canvas( is your intent to measure the canvas creation cost? I think not... at which point, the way you had the timing inside the loop was more correct. Also, we should be using TImeticks::highresnow for our timings I think. @jbates will know.
On Wed, Aug 15, 2012 at 10:36 AM, <nduca@chromium.org> wrote: > LGTM but please resolve concerns below > > > https://chromiumcodereview.**appspot.com/10837223/diff/** > 7004/content/renderer/all_**rendering_benchmarks.cc<https://chromiumcodereview.appspot.com/10837223/diff/7004/content/renderer/all_rendering_benchmarks.cc> > File content/renderer/all_**rendering_benchmarks.cc (right): > > https://chromiumcodereview.**appspot.com/10837223/diff/** > 7004/content/renderer/all_**rendering_benchmarks.cc#**newcode158<https://chromiumcodereview.appspot.com/10837223/diff/7004/content/renderer/all_rendering_benchmarks.cc#newcode158> > content/renderer/all_**rendering_benchmarks.cc:158: scoped_ptr<WebCanvas> > canvas( > is your intent to measure the canvas creation cost? I think not... at > which point, the way you had the timing inside the loop was more > correct. > I had him change it. Why wouldn't you want to count the canvas creation cost? > > Also, we should be using TImeticks::highresnow for our timings I think. > @jbates will know. > > https://chromiumcodereview.**appspot.com/10837223/<https://chromiumcodereview... >
This micro is trying to figure out just the rasterization times when tiling. The canvas costs are another micro-benchmark component that we could time separately if we were interested. In this case, I dont think we're interested because, Iirw, @reveman added some code that pools our canvases so we don't pay to create them for every tile.
https://chromiumcodereview.appspot.com/10837223/diff/7004/content/renderer/al... File content/renderer/all_rendering_benchmarks.cc (right): https://chromiumcodereview.appspot.com/10837223/diff/7004/content/renderer/al... content/renderer/all_rendering_benchmarks.cc:158: scoped_ptr<WebCanvas> canvas( On 2012/08/15 17:36:48, nduca wrote: > is your intent to measure the canvas creation cost? I think not... at which > point, the way you had the timing inside the loop was more correct. > > Also, we should be using TImeticks::highresnow for our timings I think. @jbates > will know. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmurph@chromium.org/10837223/12004
Change committed as 151783 |
