Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
186973005:
New version of the bbh shootout (Closed)
|
Created:
6 years, 9 months ago by iancottrell Modified:
6 years, 9 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionNew version of the bbh shootout.
There were a bunch of problems with the existing code, all of which I would of ignored, except it could only benchmark none and rtree.
The new version measures the same numbers, in almost exactly the same way, it's the structure over the top of the actual test runner that I changed.
Now it only loads the pictures once, it is configurable in which bbh's it runs and how may loops on them it does, it uses standard command line processing, and it does not do so much redundant work so it runs quicker. It is also much easier to understand and change IMHO.
Doing this so I can reasonably compare the new QuadTree implementation.
BUG=skia:2242
Committed: http://code.google.com/p/skia/source/detail?r=13736
Committed: http://code.google.com/p/skia/source/detail?r=13743
Patch Set 1 #Patch Set 2 : Remove dead constants #
Total comments: 28
Patch Set 3 : Review fixes #
Total comments: 8
Patch Set 4 : Removing verification tests, and minor fixes #Patch Set 5 : Clean up do_benchmark_work args and comments #Patch Set 6 : Remove accidentally added file" #Patch Set 7 : Fix bbh_shootout break for mac #Messages
Total messages: 19 (0 generated)
api lgtm https://codereview.chromium.org/186973005/diff/20001/tools/PictureRenderer.h File tools/PictureRenderer.h (right): https://codereview.chromium.org/186973005/diff/20001/tools/PictureRenderer.h#... tools/PictureRenderer.h:89: static const int kBBoxHierarchyTypeCount = kLast_BBoxHierarchyType + 1; stylistically, I'm not a fan of these in public headers.
https://codereview.chromium.org/186973005/diff/20001/tools/PictureRenderer.h File tools/PictureRenderer.h (right): https://codereview.chromium.org/186973005/diff/20001/tools/PictureRenderer.h#... tools/PictureRenderer.h:89: static const int kBBoxHierarchyTypeCount = kLast_BBoxHierarchyType + 1; If you can get away with it, I'd recommend using only the Count. Having kLast_Thing can make compilers warnings get annoying about non-exhaustive switches. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:11: #include "PictureRenderer.h" Are you happy using PictureBenchmark/PictureRenderer/etc? I don't want to rock the boat if you are, but I've always found that code harder to work with than just writing it from scratch, like bench_record. I can't help but read this file and see most of the complexity coming from those classes. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:24: SkString fName; This is just my opinion, but for a struct with no methods, I find it clearer to drop the fPrefix. struct Measurement { SkString name; SkScalar recordAverage[kBBoxTypeCount]; SkScalar playbackAverage[kBBoxTypeCount]; }; There's no way for name to be ambiguous because it never stands alone: it's always got to foo.name or bar->name. Though, sometimes this raises the question, does this struct really have no methods, or perhaps are they just hiding in other code? https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:30: "normal", // kNone_BBoxHierarchyType normal -> none? https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:54: renderer.render(&path, &bitmap); Man, this has an odd signature. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:71: static void do_benchmark_work(sk_tools::PictureRenderer* renderer, Do you mind arranging these guys one-per-line? When there are many and various arguments, it's sometimes hard to digest, or even just count how many there are. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:76: renderer->setGridSize(512, 512); Make this a flag defaulting to 512? https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:111: if (!compare_picture(path, *bitmap, pic)) { A correctness test seems harmless but out of place here. What do you think? https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:118: DEFINE_string(bbh, "", "The list of SKPs to benchmark."); Probably not the right comment. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:127: usage.printf("Usage: filename [filename]*\n"); I think SkCommandLineFlags has a built-in usager. Seems like this usage information is actually out of date anyway? https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:151: if (FLAGS_playback) { Can you tack on the > 0 for each of these? First time I read through I missed that they were ints. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:168: Measurement globalMeasurement; This chunk might be easier to read if you renamed this global. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:180: globalMeasurement.fPlaybackAverage[bBoxType] /= measurements.count(); This is the true global average, right, as every SKP loops the same number of times?
On 2014/03/07 13:39:48, reed1 wrote: > api lgtm > > https://codereview.chromium.org/186973005/diff/20001/tools/PictureRenderer.h > File tools/PictureRenderer.h (right): > > https://codereview.chromium.org/186973005/diff/20001/tools/PictureRenderer.h#... > tools/PictureRenderer.h:89: static const int kBBoxHierarchyTypeCount = > kLast_BBoxHierarchyType + 1; > stylistically, I'm not a fan of these in public headers. tools/ is meant to be public?
On 2014/03/07 13:41:53, mtklein wrote: > On 2014/03/07 13:39:48, reed1 wrote: > > api lgtm > > > > https://codereview.chromium.org/186973005/diff/20001/tools/PictureRenderer.h > > File tools/PictureRenderer.h (right): > > > > > https://codereview.chromium.org/186973005/diff/20001/tools/PictureRenderer.h#... > > tools/PictureRenderer.h:89: static const int kBBoxHierarchyTypeCount = > > kLast_BBoxHierarchyType + 1; > > stylistically, I'm not a fan of these in public headers. > > tools/ is meant to be public? public as opposed to protected/private.
https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:24: SkString fName; On 2014/03/07 13:41:15, mtklein wrote: > This is just my opinion, but for a struct with no methods, I find it clearer to > drop the fPrefix. > struct Measurement { > SkString name; > SkScalar recordAverage[kBBoxTypeCount]; > SkScalar playbackAverage[kBBoxTypeCount]; > }; > > There's no way for name to be ambiguous because it never stands alone: it's > always got to foo.name or bar->name. > > Though, sometimes this raises the question, does this struct really have no > methods, or perhaps are they just hiding in other code? For me, the fPrefix is nice to always use... esp. since simple structs can add methods later on, and often do.
https://codereview.chromium.org/186973005/diff/20001/tools/PictureRenderer.h File tools/PictureRenderer.h (right): https://codereview.chromium.org/186973005/diff/20001/tools/PictureRenderer.h#... tools/PictureRenderer.h:89: static const int kBBoxHierarchyTypeCount = kLast_BBoxHierarchyType + 1; On 2014/03/07 13:41:15, mtklein wrote: > If you can get away with it, I'd recommend using only the Count. Having > kLast_Thing can make compilers warnings get annoying about non-exhaustive > switches. Removed the count. kLast seemed to be the standard across all of skia, I though all compilers had been fixed for those exhaustive messages now? https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:11: #include "PictureRenderer.h" On 2014/03/07 13:41:15, mtklein wrote: > Are you happy using PictureBenchmark/PictureRenderer/etc? I don't want to rock > the boat if you are, but I've always found that code harder to work with than > just writing it from scratch, like bench_record. I can't help but read this > file and see most of the complexity coming from those classes. Did not even look at it. I just re-arranged the top logic in this file, tried to stay away from changing the actual benchmark part. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:24: SkString fName; On 2014/03/07 13:58:20, reed1 wrote: > On 2014/03/07 13:41:15, mtklein wrote: > > This is just my opinion, but for a struct with no methods, I find it clearer > to > > drop the fPrefix. > > struct Measurement { > > SkString name; > > SkScalar recordAverage[kBBoxTypeCount]; > > SkScalar playbackAverage[kBBoxTypeCount]; > > }; > > > > There's no way for name to be ambiguous because it never stands alone: it's > > always got to foo.name or bar->name. > > > > Though, sometimes this raises the question, does this struct really have no > > methods, or perhaps are they just hiding in other code? > > For me, the fPrefix is nice to always use... esp. since simple structs can add > methods later on, and often do. I will leave this. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:30: "normal", // kNone_BBoxHierarchyType On 2014/03/07 13:41:15, mtklein wrote: > normal -> none? An attempt to stay compatibly with what the previous benchmark implementation printed. Changed it. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:54: renderer.render(&path, &bitmap); On 2014/03/07 13:41:15, mtklein wrote: > Man, this has an odd signature. Yes, it really is. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:71: static void do_benchmark_work(sk_tools::PictureRenderer* renderer, On 2014/03/07 13:41:15, mtklein wrote: > Do you mind arranging these guys one-per-line? When there are many and various > arguments, it's sometimes hard to digest, or even just count how many there are. Done. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:76: renderer->setGridSize(512, 512); On 2014/03/07 13:41:15, mtklein wrote: > Make this a flag defaulting to 512? Done. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:111: if (!compare_picture(path, *bitmap, pic)) { On 2014/03/07 13:41:15, mtklein wrote: > A correctness test seems harmless but out of place here. What do you think? Yeah, I think it's pointless, only left it because I was trying to not modify behavior too much, I will rip it out. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:118: DEFINE_string(bbh, "", "The list of SKPs to benchmark."); On 2014/03/07 13:41:15, mtklein wrote: > Probably not the right comment. Done. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:127: usage.printf("Usage: filename [filename]*\n"); On 2014/03/07 13:41:15, mtklein wrote: > I think SkCommandLineFlags has a built-in usager. Seems like this usage > information is actually out of date anyway? Removed. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:151: if (FLAGS_playback) { On 2014/03/07 13:41:15, mtklein wrote: > Can you tack on the > 0 for each of these? First time I read through I missed > that they were ints. Done. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:168: Measurement globalMeasurement; On 2014/03/07 13:41:15, mtklein wrote: > This chunk might be easier to read if you renamed this global. Done. https://codereview.chromium.org/186973005/diff/20001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:180: globalMeasurement.fPlaybackAverage[bBoxType] /= measurements.count(); On 2014/03/07 13:41:15, mtklein wrote: > This is the true global average, right, as every SKP loops the same number of > times? Yes I was wondering if we actually want an averaged percentage difference from baseline number as well, as at the moment the long running tests dominate the avereage, but I would rather add that as a separate cl if we want it.
> Yes > I was wondering if we actually want an averaged percentage difference from > baseline number as well, as at the moment the long running tests dominate the > avereage, but I would rather add that as a separate cl if we want it. Yeah, I regularly pay attention only to the geometric mean when I'm aggregating metrics in bench_pictures and bench_record because of this.
Are we waiting for any other changes here, Ian?
On 2014/03/10 14:44:34, tomhudson wrote: > Are we waiting for any other changes here, Ian? No, this is ready to go as far as I am concerned.
lgtm https://codereview.chromium.org/186973005/diff/40001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/186973005/diff/40001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:28: DEFINE_int32(playback, 1, "Number of times to playback each SKP."); The record/playback ratio feels like the opposite of what we see in practice. Do we really want to bias this test that strongly? https://codereview.chromium.org/186973005/diff/40001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:29: DEFINE_int32(tilesize, 512, "The size of a tile."); Not quite a nit: in Chromium, cc/trees/layer_tree_settings.h still defaults to 256 despite our arguments they should increase it. Also, apparently Chromium hybrid rendering with Ganesh uses 384 or 416. But none of the embedders we know about use 512, so it might not be the best default setting. :( https://codereview.chromium.org/186973005/diff/40001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:74: * Renders the picture into the renderer. Nit: I hate comments that use the same root twice in two different word forms because their meaning can be really unclear. Or tautological. Not to mention "picture" is misleading here... https://codereview.chromium.org/186973005/diff/40001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:85: bool verify) { Nit: is there a more descriptive name? verifyWhat?
https://codereview.chromium.org/186973005/diff/40001/tools/bbh_shootout.cpp File tools/bbh_shootout.cpp (right): https://codereview.chromium.org/186973005/diff/40001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:28: DEFINE_int32(playback, 1, "Number of times to playback each SKP."); On 2014/03/10 14:59:15, tomhudson wrote: > The record/playback ratio feels like the opposite of what we see in practice. Do > we really want to bias this test that strongly? We don't build a combined metric, so the ratio does not really matter, and all reported numbers are average for a single iteration. The record is over 100 times faster than the playback, seemed fair to spend about the same amount of time on each. https://codereview.chromium.org/186973005/diff/40001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:29: DEFINE_int32(tilesize, 512, "The size of a tile."); On 2014/03/10 14:59:15, tomhudson wrote: > Not quite a nit: in Chromium, cc/trees/layer_tree_settings.h still defaults to > 256 despite our arguments they should increase it. > > Also, apparently Chromium hybrid rendering with Ganesh uses 384 or 416. > > But none of the embedders we know about use 512, so it might not be the best > default setting. :( Done. https://codereview.chromium.org/186973005/diff/40001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:74: * Renders the picture into the renderer. On 2014/03/10 14:59:15, tomhudson wrote: > Nit: I hate comments that use the same root twice in two different word forms > because their meaning can be really unclear. Or tautological. Not to mention > "picture" is misleading here... Me too, but it was not my comment :) I have changed the signature and all the comment to be much clearer, I hope. https://codereview.chromium.org/186973005/diff/40001/tools/bbh_shootout.cpp#n... tools/bbh_shootout.cpp:85: bool verify) { On 2014/03/10 14:59:15, tomhudson wrote: > Nit: is there a more descriptive name? verifyWhat? Removed! The job of this tool is to benchmark, not to verify correctness.
The CQ bit was checked by tomhudson@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/iancottrell@google.com/186973005/100001
Message was sent while issue was closed.
Change committed as 13736
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/194903003/ by halcanary@google.com. The reason for reverting is: breaking build on at least 12 configurations..
The CQ bit was checked by iancottrell@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/iancottrell@google.com/186973005/120001
Message was sent while issue was closed.
Change committed as 13743 |
