Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16948011:
Measure tiled rendering. (Closed)
|
Created:
7 years, 6 months ago by sglez Modified:
7 years, 5 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionMeasurement tool for Bounding-Box-Hierarchies.
BUG=
Committed: http://code.google.com/p/skia/source/detail?r=10173
Patch Set 1 #Patch Set 2 : Measure recording and playback. Baseline vs Rtree #Patch Set 3 : Getting results... #
Total comments: 18
Patch Set 4 : Code review #
Total comments: 6
Patch Set 5 : Histogram, code review #Patch Set 6 : Remove old constructor #
Total comments: 4
Patch Set 7 : Applied suggestions from talk. Avoid IO operations. Support for different tile sizes #
Total comments: 24
Patch Set 8 : Code review fixes #
Total comments: 38
Patch Set 9 : Struct to handle benchmark info. Other code review tweaks. #
Total comments: 1
Patch Set 10 : Remove profanity #Patch Set 11 : Remove printing of results to stdout. #Patch Set 12 : SkScalar casts #
Messages
Total messages: 21 (0 generated)
Would you please take a look at my measurement tool? bbh_filter.lua is used with lua_pictures to get interesting files. bbh_shootout.cpp uses two renderers: RecordPictureRenderer and TiledPlaybackRenderer. It finds N such that playing a picture N times using an RTree is faster than playing it N times without. i.e. rtree_recording_time + (N * rtree_playback_time) < base_recording_time + (N * base_playback_time) For every sample of files I can conjure, RTree seems like a good choice. N is always very small. So naturally I must be doing something wrong! Details: Usually it takes from 10% to 50% longer to record a picture with an RTree but on average the 1%-5% return in playback speed seems to be worth it. I have double-checked the code and stepped through with a debugger to make sure I am measuring what I want to measure.
Looks good -- most of my comments are nits. https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:45: return pic; just let it fall through https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:78: for(int i = 0; i < numRepeats; ++i) { space after for https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:81: if(!result) { space after if https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:92: SkAutoTUnref<sk_tools::TiledPictureRenderer> renderer(SkNEW(sk_tools::TiledPictureRenderer)); Since this just lives on the stack, do you need to allocate it? Would sk_tools::TiledPictureRender renderer; work as well? https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:120: /*showTruncatedCpuTime=*/false, /*showGpuTime=*/false Good documentation, if a little hard to read. Use more spaces, e.g. /* logPerIter= */ false, and maybe it makes sense to put each parameter on its on line, to make it easier to visually parse. https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:123: int pos = timerResult.find(findStr); error check if find fails? https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:124: SkDebugf("%s\n", timerResult.c_str()); probably should put debug before find in case find fails https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:125: const SkScalar cpuTime = atof(timerResult.c_str() + pos + sizeof(findStr) - 1); const doesn't add any information here https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:126: return cpuTime; error check atof ? https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:129: static const SkString perIterTimeFormat = SkString("%f"); ... perIterTimeFormat("%f"); etc. https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:143: for(int index = 1; index < argc; ++index) { space after for https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:145: const SkString path = SkString(argv[index]); path(argv[index]) https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:147: SkAutoTUnref<SkPicture> pic(pic_from_path(argv[index])); no auto unref required, I don't think, e.g. SkPicture pic(...); https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:155: int tool_main(int argc, char** argv); is this seen by someone external to this file, i.e., could it be static? https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:177: const SkScalar rtreePlaybackResult = get_data_result(rtreePlaybackData, "rtree_playback"); not sure const adds any value here
https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:92: SkAutoTUnref<sk_tools::TiledPictureRenderer> renderer(SkNEW(sk_tools::TiledPictureRenderer)); On 2013/06/28 18:09:43, caryclark wrote: > Since this just lives on the stack, do you need to allocate it? > > Would > > sk_tools::TiledPictureRender renderer; > > work as well? Changed it to live on the stack. https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:147: SkAutoTUnref<SkPicture> pic(pic_from_path(argv[index])); On 2013/06/28 18:09:43, caryclark wrote: > no auto unref required, I don't think, e.g. > > SkPicture pic(...); pic_from_path was returning a pointer. I modified the function. https://codereview.chromium.org/16948011/diff/4001/tools/bbh_shootout.cpp#new... tools/bbh_shootout.cpp:155: int tool_main(int argc, char** argv); On 2013/06/28 18:09:43, caryclark wrote: > is this seen by someone external to this file, i.e., could it be static? I was following the crowd. Everyone defines tool_main as external. I can't find a reason why, though... Changed it to static. Compiles/Runs without problems on my Mac and Linux machines
https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:42: if (!success) { Is there a Factory method to create a picture from a stream? https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:178: TimerData normalRecordData = Can this section be organized w/ tables of static data somehow? It looks like a lot of copy/paste, which makes it hard to check, and harder to edit in the future.
https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:166: return timerData; instead of returning the timerData, why not return the get_data_result ?
Added histograms https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:42: if (!success) { On 2013/07/08 12:07:03, reed1 wrote: > Is there a Factory method to create a picture from a stream? There is SkPicture::CreateFromStream... it's newer than this code. It seems to do the same thing as this constructor, but it returns a pointer and we are keeping these pictures in the stack. Also, we still need to check that the stream creation does not fail. Maybe write a SkPicture::CreateFromPath? https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:166: return timerData; On 2013/07/08 13:04:57, caryclark wrote: > instead of returning the timerData, why not return the get_data_result ? Moved the body of get_data_result into this function https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:178: TimerData normalRecordData = On 2013/07/08 12:07:03, reed1 wrote: > Can this section be organized w/ tables of static data somehow? It looks like a > lot of copy/paste, which makes it hard to check, and harder to edit in the > future. Done. Thanks.
FYI -- The latest patch is broken because the SkPicture constructor I was using was removed. I have a fix but I can't upload it from home. On 2013/07/08 22:00:34, sglez wrote: > Added histograms > > https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp > File tools/bbh_shootout.cpp (right): > > https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp#ne... > tools/bbh_shootout.cpp:42: if (!success) { > On 2013/07/08 12:07:03, reed1 wrote: > > Is there a Factory method to create a picture from a stream? > > There is SkPicture::CreateFromStream... it's newer than this code. It seems to > do the same thing as this constructor, but it returns a pointer and we are > keeping these pictures in the stack. Also, we still need to check that the > stream creation does not fail. > > Maybe write a SkPicture::CreateFromPath? > > https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp#ne... > tools/bbh_shootout.cpp:166: return timerData; > On 2013/07/08 13:04:57, caryclark wrote: > > instead of returning the timerData, why not return the get_data_result ? > > Moved the body of get_data_result into this function > > https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp#ne... > tools/bbh_shootout.cpp:178: TimerData normalRecordData = > On 2013/07/08 12:07:03, reed1 wrote: > > Can this section be organized w/ tables of static data somehow? It looks like > a > > lot of copy/paste, which makes it hard to check, and harder to edit in the > > future. > > Done. Thanks.
Uploaded the patch... Sorry about the triple email! On 2013/07/09 02:12:41, sglez wrote: > FYI -- The latest patch is broken because the SkPicture constructor I was using > was removed. I have a fix but I can't upload it from home. > > On 2013/07/08 22:00:34, sglez wrote: > > Added histograms > > > > https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp > > File tools/bbh_shootout.cpp (right): > > > > > https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp#ne... > > tools/bbh_shootout.cpp:42: if (!success) { > > On 2013/07/08 12:07:03, reed1 wrote: > > > Is there a Factory method to create a picture from a stream? > > > > There is SkPicture::CreateFromStream... it's newer than this code. It seems to > > do the same thing as this constructor, but it returns a pointer and we are > > keeping these pictures in the stack. Also, we still need to check that the > > stream creation does not fail. > > > > Maybe write a SkPicture::CreateFromPath? > > > > > https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp#ne... > > tools/bbh_shootout.cpp:166: return timerData; > > On 2013/07/08 13:04:57, caryclark wrote: > > > instead of returning the timerData, why not return the get_data_result ? > > > > Moved the body of get_data_result into this function > > > > > https://codereview.chromium.org/16948011/diff/16001/tools/bbh_shootout.cpp#ne... > > tools/bbh_shootout.cpp:178: TimerData normalRecordData = > > On 2013/07/08 12:07:03, reed1 wrote: > > > Can this section be organized w/ tables of static data somehow? It looks > like > > a > > > lot of copy/paste, which makes it hard to check, and harder to edit in the > > > future. > > > > Done. Thanks.
Here is a patch with fixes from the last code-review. Also added some functionality and applied suggestions from yesterday's talk. Thanks!
https://codereview.chromium.org/16948011/diff/31001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/16948011/diff/31001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:16: #include "SkGraphics.h" alphabetize https://codereview.chromium.org/16948011/diff/31001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:27: static const int gTileDimensions[kNumTileDimensions][2] = { use [] instead of [2] https://codereview.chromium.org/16948011/diff/31001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:39: SkScalar cpuTime; fPathIndex fCpuTime https://codereview.chromium.org/16948011/diff/31001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:168: return -1.0; this may generate a warning since it's a double cast to a float. Use SkDoubleToScalar(-1.0) or SkIntToScalar(-1) to avoid the warning. https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:17: #include "SkGraphics.h" as before, alphabetize https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:62: msgPrefix.printf("Normal"); this may generate warnings on some platforms, that expect printf("%s", "string") instead. Until you need the flexibility of printf, use set() instead. https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:68: break; default: SkASSERT(0); https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:134: void (*func)(BenchmarkType, const int[], const SkString*, SkPicture*, BenchTimer*), skia's convention is to pass const SkString& https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:150: histogram[index - 1].cpuTime = timer.fCpu; if the pic == NULL, what is timer.fCpu? Does it make sense to record data for pictures that fail to decode? If it does, put some comments here to note your logic. https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:175: return -1.0; return SkIntToScalar(-1); https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:194: }; this seems unnecessarily fragile and will make adding a new tile size tricky. Start by defining kNumTileSizes as static const size_t kNumTileSizes = SK_ARRAY_COUNT(tileSizes); https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:195: static const int kNumBenchmarks = 6; static const size_t kNumBenchmarks = 3 + kNumTileSizes; https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:202: "rtree_playback_1024x1024", build these rtree_playback strings from the tile sizes https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:212: }; defer array initialization until you use it below https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:217: benchmark_recording, // rtree_recording change this to only contain the first 3 entries so that tile size can be extendable. If this array is moved above kNumBenchmarks then the '3' in the comment above and below could be replaced by SK_ARRAY_COUNT(benchmarkFunctions); (maybe rename to baseBenchmarkFunctions to distinguish from playback functions with different tile sizes?) https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:234: tileSize[1] = tileSizes[i - 3][1]; add benchmarkFunction = benchmark_playback; https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:235: } add histograms[i] = SkNEW_ARRAY(... https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:268: SkScalar minMax[][2] = { minMax[kNumBenchmarks][... then remove initialization https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:278: for (int j = 0; j < kNumBenchmarks; ++j) { reverse loop nesting so you can initialize minMax dynamically https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:288: for (int i = 0; i < kNumBenchmarks; ++i) { reversing the loop nesting above allows this to be part of the outer loop https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:294: SkDebugf("%s", out.c_str()); is this more clear to you than if you replaced the printf and appendf with SkDebugf ? https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:316: for (int j = 3; j < kNumBenchmarks; ++j) { replace 3 with some const defined above. https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:327: SkDELETE(histograms[i]); this suggests that some auto use is missing here
https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:62: msgPrefix.printf("Normal"); On 2013/07/10 15:28:24, caryclark wrote: > this may generate warnings on some platforms, that expect > printf("%s", "string") > instead. > Until you need the flexibility of printf, use set() instead. Thanks, I didn't know about that. https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:134: void (*func)(BenchmarkType, const int[], const SkString*, SkPicture*, BenchTimer*), On 2013/07/10 15:28:24, caryclark wrote: > skia's convention is to pass const SkString& Sorry about that, I changed some more cases where input params should be const TYPE& https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:150: histogram[index - 1].cpuTime = timer.fCpu; On 2013/07/10 15:28:24, caryclark wrote: > if the pic == NULL, what is timer.fCpu? Does it make sense to record data for > pictures that fail to decode? If it does, put some comments here to note your > logic. This was more a lack of logic on my part than anything else. There is a need to set the cpuTime to an invalid value in the case that the picture is null. https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:294: SkDebugf("%s", out.c_str()); On 2013/07/10 15:28:24, caryclark wrote: > is this more clear to you than if you replaced the printf and appendf with > SkDebugf ? SkDebugf is much clearer https://codereview.chromium.org/16948011/diff/40001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:327: SkDELETE(histograms[i]); On 2013/07/10 15:28:24, caryclark wrote: > this suggests that some auto use is missing here I am now using SkTArrays
https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:30: struct Histogram { Add Histogram() : fCpuTime(SkIntToScalar(-1)) { } so it's always initialized to the invalid state (and comment it too) https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:36: extern bool lazy_decode_bitmap(const void* buffer, size_t size, SkBitmap* bitmap); since it's also used by bench_pictures_main.cpp, lua_pictures.cpp, and render_pictures_main.cpp, it's time to move this to tools/PictureRenderFlags.h (it also needs to be renamed to lazyDecodeBitmap, since in its present form, it looks like a local static function) Since this change is orthogonal to the shootout, prepare a separate CL that just does the rename/add to .h, and (after running it through the trybots) I'll approve it immediately https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:92: SkDebugf("Error rendering (playback).\n"); If recording == false, this will print the debug message each time through the loop, whether render() returns true or not. Do the check outside the loop instead. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:136: void (*func)(BenchmarkType, const int[], const SkString&, SkPicture*, BenchTimer*), below, you use const int[kNumBenchMarks]. Make these consistent. I recommend moving your const data to the top of the file, so you can reference any computed consts consistently. Also, since this func shows up more than once, I recommend using a typedef so that all uses are always the same. Name the typedef so that the purpose of the function pointer is self-documenting. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:137: const int tileSize[2], I assume the '2' is the number of tile dimensions, i.e., it's a 2D tile with a width and height. Replacing the 2 with a good const name would make this more clear. In general, I recommend avoiding any numbers other than 1 and 0 outside of auto initialization unless it's awkward (I wouldn't, for instance, name a number and then just use the number once). I also wouldn't change the if (argc < 2) { below because it would obscure rather than enlighten the code -- in other words, use your best judgment. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:142: for (int index = 1; index < argc; ++index) { this seems inconsistent. If argc == 1, this loop executes once. However, the check in tool_main requires argc to be 2 or larger. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:147: SkDebugf("Couldn't create picture. Ignoring path: %s\n", path.c_str()); Here you use path.c_str() for char* . A couple lines up you use argv[index] as an argument to pic_from_path for char* . Pick one. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:149: histogram[index - 1].fCpuTime = SkIntToScalar(-1); move this to the struct constructor (see above) https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:152: func(benchmarkType, tileSize, path, pic, &timer); (*func)(benchmarkType ... https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:153: timerData.appendTimes(&timer, index == argc - 1); this is a good place to reverse this to argc - 1 == index since argc - 1 = index won't compile https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:179: if (cpuTime == SkIntToScalar(0)) { // atof returns 0.0 on error. 0 is always 0, so you can say if (cpuTime == 0) { regardless of whether cpuTime's type. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:192: SkDebugf("%s\n", usage.c_str()); return something other than zero in the error case? https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:202: static const size_t kNumBenchmarks = 3 + kNumTileSizes; you want kNumBaseBenches (or another name of your choice) instead of 3 -- see below https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:210: benchName.printf("rtree_playback_%dx%d", tileSizes[i][0], tileSizes[i][1]); earlier, you set the name in the string and did appendf("_%dx%d"). Either pattern is fine -- choose one. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:211: benchNames[i + kNumTileSizes] = benchName.c_str(); kNumTileSizes isn't what you want here. You need another const that describes kNumBaseBenches or something like that, for bench 0, 1, and 2. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:221: }; this is one possibility for const int kNumBaseBenches = SK_ARRAY_COUNT(baseBenchmarkFunctions); https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:228: type = kRTree_BenchmarkType; I'm not crazy about benchNames being initialized in one place, baseBenchmarkFunctions in a second, and this in a third. I'd rather see a struct with all three, and then an auto-initialized array that ensures that everything is in sync. Along with this, add an enum that replaces your use of 0,1,2 (and sometimes 3) with kNormalRecording, kNormalPlayback, etc. Talk to me if this isn't clear. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:239: tileSize[1] = tileSizes[i - 3][1]; lots of '3's here :) https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:269: (kNumRecordings * (rtreePlaybackResult - normalPlaybackResult)); what about if rtreePlaybackResult == normalPlaybackResult ? https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:276: SkScalar minMax[kNumBenchmarks][2]; 2 being one for min, one for max. Good place for an enum enum { kMinBenchMarkTime, kMaxBenchMarkTime }; const int kNumBenchMarkTimes = kMaxBenchMarkTime + 1; or const int kNumBenchMarkTimes = 2; https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:286: minMax[i][0] = value; I prefer the pattern of if (a op b) { a = b; but that may be a matter of taste https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:320: pbLine.appendf("%f ", histograms[j][i].fCpuTime); this puts a final ' ' followed by a '\n'
https://codereview.chromium.org/16948011/diff/5629499534213120/tools/lua/bbh_... File tools/lua/bbh_filter.lua (right): https://codereview.chromium.org/16948011/diff/5629499534213120/tools/lua/bbh_... tools/lua/bbh_filter.lua:8: -- have most commands, and the names of the files that use the least popular commands. Since you seem to be tracking glyphs (why?) should you mention that too in this comment? https://codereview.chromium.org/16948011/diff/5629499534213120/tools/lua/bbh_... tools/lua/bbh_filter.lua:130: for _,g in pairs(globalInfo[t.fileName].glyphs) do glyphCount = glyphCount + 1 end perhaps we write a global helper function called count_entries or something, to help document what this loop is doing. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/lua/bbh_... tools/lua/bbh_filter.lua:139: io.write("\t},\n") Can these multiple io.write calls be smashed into a single call? (just for readability)
Addressed most comments. Did some refactoring to make it clearer and more extendable. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:36: extern bool lazy_decode_bitmap(const void* buffer, size_t size, SkBitmap* bitmap); On 2013/07/12 13:13:28, caryclark wrote: > since it's also used by bench_pictures_main.cpp, lua_pictures.cpp, and > render_pictures_main.cpp, it's time to move this to tools/PictureRenderFlags.h > > (it also needs to be renamed to lazyDecodeBitmap, since in its present form, it > looks like a local static function) > > Since this change is orthogonal to the shootout, prepare a separate CL that just > does the rename/add to .h, and (after running it through the trybots) I'll > approve it immediately CL sent https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:92: SkDebugf("Error rendering (playback).\n"); On 2013/07/12 13:13:28, caryclark wrote: > If recording == false, this will print the debug message each time through the > loop, whether render() returns true or not. Do the check outside the loop > instead. RecordPictureRenderer::render(..), returns false when it fails to write to a file. We expect this when we are recording because we pass NULL. (It still does the job we want it to do) TiledPictureRenderer::render(..) returns true unless something bad happens. So if recording == false, result will hopefully be true and we won't print. I don't know why they behave differently with NULL input. Perhaps RecordPictureRenderer has the assumption that it is only useful when writing to a file? I changed 'recording' to 'isPlayback', to make this code clearer. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:136: void (*func)(BenchmarkType, const int[], const SkString&, SkPicture*, BenchTimer*), On 2013/07/12 13:13:28, caryclark wrote: > below, you use const int[kNumBenchMarks]. Make these consistent. > > I recommend moving your const data to the top of the file, so you can reference > any computed consts consistently. Also, since this func shows up more than once, > I recommend using a typedef so that all uses are always the same. > > Name the typedef so that the purpose of the function pointer is > self-documenting. Yes, I should have typedef'd that some time ago. Moved most constants to the top. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:137: const int tileSize[2], On 2013/07/12 13:13:28, caryclark wrote: > I assume the '2' is the number of tile dimensions, i.e., it's a 2D tile with a > width and height. Replacing the 2 with a good const name would make this more > clear. > > In general, I recommend avoiding any numbers other than 1 and 0 outside of auto > initialization unless it's awkward (I wouldn't, for instance, name a number and > then just use the number once). I also wouldn't change the > > if (argc < 2) { > > below because it would obscure rather than enlighten the code -- in other words, > use your best judgment. In this instance, I think having a struct would be clearer. After some searching, using SkISize seems like the right thing to do. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:142: for (int index = 1; index < argc; ++index) { On 2013/07/12 13:13:28, caryclark wrote: > this seems inconsistent. If argc == 1, this loop executes once. However, the > check in tool_main requires argc to be 2 or larger. If argc == 1 this loop doesn't excecute :) https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:228: type = kRTree_BenchmarkType; On 2013/07/12 13:13:28, caryclark wrote: > I'm not crazy about benchNames being initialized in one place, > baseBenchmarkFunctions in a second, and this in a third. I'd rather see a struct > with all three, and then an auto-initialized array that ensures that everything > is in sync. > > Along with this, add an enum that replaces your use of 0,1,2 (and sometimes 3) > with kNormalRecording, kNormalPlayback, etc. > > Talk to me if this isn't clear. This comment made me do a big change. I hope I was more careful this time. It looks a lot better to me now. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:269: (kNumRecordings * (rtreePlaybackResult - normalPlaybackResult)); On 2013/07/12 13:13:28, caryclark wrote: > what about if rtreePlaybackResult == normalPlaybackResult ? I missed this comment and didn't address it. I will fix this for the next patch. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:286: minMax[i][0] = value; On 2013/07/12 13:13:28, caryclark wrote: > I prefer the pattern of > > if (a op b) { > a = b; > > but that may be a matter of taste Normally I would agree, but when doing min/max I prefer saying: "If the value is smaller than the minimum, set the minimum to be the value." than to say: "If the minimum is greater than the value, set the minimum to be the value." The first one is clearer to me. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:320: pbLine.appendf("%f ", histograms[j][i].fCpuTime); On 2013/07/12 13:13:28, caryclark wrote: > this puts a final ' ' followed by a '\n' Added a line that trims the trailing space. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/lua/bbh_... File tools/lua/bbh_filter.lua (right): https://codereview.chromium.org/16948011/diff/5629499534213120/tools/lua/bbh_... tools/lua/bbh_filter.lua:8: -- have most commands, and the names of the files that use the least popular commands. On 2013/07/12 13:24:29, reed1 wrote: > Since you seem to be tracking glyphs (why?) should you mention that too in this > comment? Vestige of something that I thought was needed. Removing all glyph code. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/lua/bbh_... tools/lua/bbh_filter.lua:130: for _,g in pairs(globalInfo[t.fileName].glyphs) do glyphCount = glyphCount + 1 end On 2013/07/12 13:24:29, reed1 wrote: > perhaps we write a global helper function called count_entries or something, to > help document what this loop is doing. Deleted this along with the rest of the glyph code... but wrote count_entries for a similar usage below. https://codereview.chromium.org/16948011/diff/53001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/16948011/diff/53001/tools/bbh_shootout.cpp#ne... tools/bbh_shootout.cpp:146: } Woops!!! This if block was me doing printf debugging and forgetting to erase. Please ignore. Will remove
https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:92: SkDebugf("Error rendering (playback).\n"); My mistake. I misread the && for an ||. On 2013/07/13 03:57:15, sglez wrote: > On 2013/07/12 13:13:28, caryclark wrote: > > If recording == false, this will print the debug message each time through the > > loop, whether render() returns true or not. Do the check outside the loop > > instead. > > RecordPictureRenderer::render(..), returns false when it fails to write to a > file. We expect this when we are recording because we pass NULL. (It still does > the job we want it to do) > TiledPictureRenderer::render(..) returns true unless something bad happens. > > So if recording == false, result will hopefully be true and we won't print. > > I don't know why they behave differently with NULL input. Perhaps > RecordPictureRenderer has the assumption that it is only useful when writing to > a file? > > I changed 'recording' to 'isPlayback', to make this code clearer. https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... tools/bbh_shootout.cpp:142: for (int index = 1; index < argc; ++index) { Understood -- my mistake again. See, aren't code reviews fun? On 2013/07/13 03:57:15, sglez wrote: > On 2013/07/12 13:13:28, caryclark wrote: > > this seems inconsistent. If argc == 1, this loop executes once. However, the > > check in tool_main requires argc to be 2 or larger. > > If argc == 1 this loop doesn't excecute :)
On 2013/07/15 12:00:15, caryclark wrote: > https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... > File tools/bbh_shootout.cpp (right): > > https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... > tools/bbh_shootout.cpp:92: SkDebugf("Error rendering (playback).\n"); > My mistake. I misread the && for an ||. > On 2013/07/13 03:57:15, sglez wrote: > > On 2013/07/12 13:13:28, caryclark wrote: > > > If recording == false, this will print the debug message each time through > the > > > loop, whether render() returns true or not. Do the check outside the loop > > > instead. > > > > RecordPictureRenderer::render(..), returns false when it fails to write to a > > file. We expect this when we are recording because we pass NULL. (It still > does > > the job we want it to do) > > TiledPictureRenderer::render(..) returns true unless something bad happens. > > > > So if recording == false, result will hopefully be true and we won't print. > > > > I don't know why they behave differently with NULL input. Perhaps > > RecordPictureRenderer has the assumption that it is only useful when writing > to > > a file? > > > > I changed 'recording' to 'isPlayback', to make this code clearer. > > https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... > tools/bbh_shootout.cpp:142: for (int index = 1; index < argc; ++index) { > Understood -- my mistake again. See, aren't code reviews fun? > On 2013/07/13 03:57:15, sglez wrote: > > On 2013/07/12 13:13:28, caryclark wrote: > > > this seems inconsistent. If argc == 1, this loop executes once. However, the > > > check in tool_main requires argc to be 2 or larger. > > > > If argc == 1 this loop doesn't excecute :) Patch uploaded, removing the offending SkDebugf. Yes, code review is fun :)
Cleaned up the code. Friendly ping for further comments or lgtm. I could use this tool in other branches. Thanks On 2013/07/15 17:17:25, sglez wrote: > On 2013/07/15 12:00:15, caryclark wrote: > > > https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... > > File tools/bbh_shootout.cpp (right): > > > > > https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... > > tools/bbh_shootout.cpp:92: SkDebugf("Error rendering (playback).\n"); > > My mistake. I misread the && for an ||. > > On 2013/07/13 03:57:15, sglez wrote: > > > On 2013/07/12 13:13:28, caryclark wrote: > > > > If recording == false, this will print the debug message each time through > > the > > > > loop, whether render() returns true or not. Do the check outside the loop > > > > instead. > > > > > > RecordPictureRenderer::render(..), returns false when it fails to write to a > > > file. We expect this when we are recording because we pass NULL. (It still > > does > > > the job we want it to do) > > > TiledPictureRenderer::render(..) returns true unless something bad happens. > > > > > > So if recording == false, result will hopefully be true and we won't print. > > > > > > I don't know why they behave differently with NULL input. Perhaps > > > RecordPictureRenderer has the assumption that it is only useful when writing > > to > > > a file? > > > > > > I changed 'recording' to 'isPlayback', to make this code clearer. > > > > > https://codereview.chromium.org/16948011/diff/5629499534213120/tools/bbh_shoo... > > tools/bbh_shootout.cpp:142: for (int index = 1; index < argc; ++index) { > > Understood -- my mistake again. See, aren't code reviews fun? > > On 2013/07/13 03:57:15, sglez wrote: > > > On 2013/07/12 13:13:28, caryclark wrote: > > > > this seems inconsistent. If argc == 1, this loop executes once. However, > the > > > > check in tool_main requires argc to be 2 or larger. > > > > > > If argc == 1 this loop doesn't excecute :) > > Patch uploaded, removing the offending SkDebugf. > Yes, code review is fun :)
lgtm Please do a trybot as before before committing, since the last time the windows try failed. Note that as of this morning, the buildbots / trybots are still completely hosed, so you should wait for build sanity before checking in.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sglez@google.com/16948011/79001
Message was sent while issue was closed.
Change committed as 10173
Message was sent while issue was closed.
This was reverted in r10174 due to compilation failures (http://108.170.217.252:10117/builders/Build-Mac10.6-GCC-x86-Debug/builds/898/...) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
