Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2880953002:
Measure frame decodes more accurately from ImageDecodeBench
DescriptionImageDecodeBench currently reports the mean without reporting standard deviation / variance. This means we cannot perfectly compare before & after measurements -- we need to make sure the central limit theorem has enough samples to maintain consistent standard deviation / variance between runs.
ImageDecodeBench will now report the standard deviation along with the mean so the user can verify that the before & after runs have a consistent amount of noise.
In order to assist the standard deviation / variance from changing much between runs, we can also increase the precision of our timing. This patch does that by starting the timer immediately before decoding a single frame, decoding the frame, and then immediately stopping the timer. It also no longer includes setup and teardown in the measurement.
Since we are now timing individual runs to find the standard deviation / variance, we also added a --raw-output command line option which saves the measurements in a csv-formatted file. This can then be used in external visualization tools.
BUG=722147
Patch Set 1 #
Total comments: 24
Patch Set 2 : Code review comment fixes #
Total comments: 2
Patch Set 3 : Rebase #Patch Set 4 : Rebase #Patch Set 5 : Add output format comment. Remove unneccesary parameter. #Patch Set 6 : Remove raster timing #
Total comments: 24
Patch Set 7 : Add comment clarifying new types. Use 'i' for loop indices. Don't specify blink:: #Patch Set 8 : Print error message and return error code when failure occurs #
Total comments: 2
Patch Set 9 : Remove ClearCacheExceptFrame & calls to SetData to reset and clear #Patch Set 10 : Clarify how output is formatted and how to use the output #
Total comments: 6
Patch Set 11 : Use 0-based index in comments. Clarify typical usage in comments #
Total comments: 1
Patch Set 12 : Create new decoder for each iteration to not allow decoder cache usage #Patch Set 13 : Add --raw-output option. Print raw timings there #Patch Set 14 : Output mean and variance #Patch Set 15 : Use standard deviation instead of variance #Patch Set 16 : Remove constexpr from variables whos usage is close in proximity #
Total comments: 10
Patch Set 17 : Fix packeted decode timing falsely erroring. Clarify comments. #Patch Set 18 : Fix std::pow since we define our own pow. Use fabs instead of std::abs #
Total comments: 15
Patch Set 19 : Report minimum #Patch Set 20 : Refactor out frame count #Patch Set 21 : Fix incorrect error message when packeted decode hits incomplete frame #Patch Set 22 : Fix treating all data received as an error in packeted decode #Patch Set 23 : Correct variable scrope in packeted decode #
Total comments: 3
Patch Set 24 : Rebase #Patch Set 25 : Fix reported minimum always being 0 #Patch Set 26 : Fix formatting #
Total comments: 6
Messages
Total messages: 88 (57 generated)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
cblume@chromium.org changed reviewers: + noel@chromium.org, scroggo@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Comments on the commit message: > Allow for more accurate measurements from ImageDecodeBench How about "Make more accurate measurements in ImageDecodeBench"? > ImageDecodeBench has a few places where it allows noise to dominate > measurements: > * It runs all iterations and divides by iteration count to get the > average. This takes long enough that the process is swapped out by the > OS. This adds noise. How long does it take for the process to be swapped out by the OS? I assume whether this is long enough depends on the number used for iterations (and the OS)? > * It includes setup and teardown in the time, > * It runs through all frames of an animated image and again averages. If > an animated image must decode a large first frame but all subsequent > frames are small sub-region updates, the average will be faster than the > slowest decode and slower than the faster decodes. It will not represent > what actually happened. For example, if decode times are improved that > will be buried under the small frame times that don't spend much time in > decode. > > To more accurately represent measurements, we should time only decode > time for each frame and report it separately. This allows us to trim out > noise and make better comparisons. Instead of stating what we "should" do, please specify what this change does - it makes it so that we *do* time and report separately the decode time for each frame. I find https://chris.beams.io/posts/git-commit/ to be helpful when writing a commit message. (It's linked to from Chromium here: https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...) It recommends using the imperative mood. It is mostly talking about for the subject line, and says "Remember: Use of the imperative is important only in the subject line. You can relax this restriction when you’re writing the body." So it can be "relaxed" in the body, but the example shown below is also written in the imperative, so I think it should still be used when appropriate. Here, for example, I think it's better to say something like "To more accurately represent measurements, start the timer immediately before decoding a single frame, decode the frame, and then immediately stop the timer. Report these times separately for each frame. This addresses the above problems: * it removes setup and teardown from the measurement * it lessens any impact of OS swapping processes * times are not skewed by differently sized frames" > Also, we should add support for timing only raster. Again, I think the imperative mood is clearer - not only should we do this, but we just did! That said, this arguably should be its own CL, with a commit message that looks something like: """ Add a mode to ImageDecodeBench that measures drawing Some changes to ImageDecoder may have different effects on the performance of decoding versus that of drawing the decoded image. Add a mode to ImageDecodeBench that measures the latter. """ https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (left): https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:325: // Warm-up: throw out the first iteration for more consistent results. Is this no longer necessary? https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:351: printf("%f %f\n", total_time, average_time); Now that we do not print the average, is the expectation that the user will do their own analysis? e.g. compute average, min, median, max? Should that be printed as part of this program? https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:8: // during image decoding on supported platforms (default off). Usage: Add comments about using --raster https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:210: void Print1DResults(const std::vector<double>& timings) { I suppose you put this here to keep it near Print2DResults? My preference would be to put it closer to where it's used. That said, it's used only once, and it seems pretty straightforward to just print the iteration time where you currently add it to the vector. https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:217: // The outer vector represents multiple iterations through the image. This comment appears several times. It makes me think the code itself should be clearer. Maybe come up with a variable name that makes it more clear? I'm not sure what that would be though. Another option: create a type that makes it more clear, with a single comment. Some ideas: // The outer vector represents multiple iterations through the image. // The inner vector represents individual frames in an animation. using FrameTime2DVector = std::vector<std::vector<double>>; or // FrameTimeVector[i] is a single timing of decoding frame i. using FrameTimeVector = std::vector<double>; // Stores FrameTimeVectors over multiple iterations. using IterationFrameTimeVector = std::vector<FrameTimeVector>; I sometimes see "using" statements in sample code and think "But then I'll have to go look up what it is when I need to use it!" But here it might be helpful, because if I do not remember what it means I look back to the "using" statement and see all I need to know. On the other hand, maybe this can just be done in line? It seems pretty straightforward - each time you add a time to the vector, you can just print it, and then print \n when you finish your iteration. Then you don't need to worry about all this explaining. https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:245: if (!frame) { As I've stated elsewhere, I don't think this can ever happen. Do you want to check GetStatus instead? https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:255: // This function mimics what actually happens in Chromium. There are different paths that result in a decode, but I think this should be more specific about when it actually happens this way. Something like: // This function mimics deferred decoding in Chromium when not all data has been received yet. https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:262: bool all_data_received = true; This variable ends up being shadowed by a variable in the outer loop, below. I don't find it necessary, though. I suppose you include it to aid readability? https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:286: size_t frame_count = decoder->FrameCount(); Since you're using the same decoder as above, I don't think this value will ever be different from total_frame_count, declared above. https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:309: // Decode first frame I'm guilty of this, too, but try to keep your comments focused on why you're doing what you're doing. Reading the code, I can see that you're decoding the first frame. That said, it's not necessarily clear to me why you only decode the first frame.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Allow for more accurate measurements from ImageDecodeBench ImageDecodeBench has a few places where it allows noise to dominate measurements: * It runs all iterations and divides by iteration count to get the average. This takes long enough that the process is swapped out by the OS. This adds noise. * It includes setup and teardown in the time, * It runs through all frames of an animated image and again averages. If an animated image must decode a large first frame but all subsequent frames are small sub-region updates, the average will be faster than the slowest decode and slower than the faster decodes. It will not represent what actually happened. For example, if decode times are improved that will be buried under the small frame times that don't spend much time in decode. To more accurately represent measurements, we should time only decode time for each frame and report it separately. This allows us to trim out noise and make better comparisons. Also, we should add support for timing only raster. BUG=722147 ========== to ========== ImageDecodeBench has a few places where it allows noise to dominate measurements: * It runs all iterations and divides by iteration count to get the average. This takes long enough that the process is swapped out by the OS. This adds noise. * It includes setup and teardown in the time, * It runs through all frames of an animated image and again averages. If an animated image must decode a large first frame but all subsequent frames are small sub-region updates, the average will be faster than the slowest decode and slower than the faster decodes. It will not represent what actually happened. For example, if decode times are improved that will be buried under the small frame times that don't spend much time in decode. To more accurately represent measurements, start the timer immediately before decoding a single frame, decode the frame, and then immediately stop the timer. Report these times separately for each frame. This addresses the above problems: * it removes setup and teardown from the measurement * it lessens any impact of OS swapping processes * times are not skewed by differently sized frames Add a mode to ImageDecodeBench that measures drawing Some changes to ImageDecoder may have different effects on the performance of decoding versus that of drawing the decoded image. Add a mode to ImageDecodeBench that measures the latter. BUG=722147 ==========
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> How long does it take for the process to be swapped out by the OS? I assume whether this is long enough depends on the number used for iterations (and the OS)? You are right, that answering it depends on a lot. The important thing is now we don't need to know those values. Since each iteration is measured separately, you will quickly see the iteration that was swapped out. You can then trim it from measurements if you choose. This same thing can be done to any source of noise. In fact, we don't care *what* the source of noise was--if we're trimming noise out, trim it from all sources. I've updated the description to be in imperative tone. I think that is a good idea. I'll be more careful about this in the future. https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (left): https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:325: // Warm-up: throw out the first iteration for more consistent results. On 2017/05/15 15:59:04, scroggo_chromium wrote: > Is this no longer necessary? If you only do 1 iteration then it'll pretty much only show you cache miss time which would have previously been discarded. But presumably the main benefit is running several iterations. If you run multiple iterations, it is perfectly okay for the first run to have a wonky time because of cache misses. It'll get tossed out if you filter noise. And you may actually want to just be aware of what to expect in the case of cache misses. Most single-frame decodes are likely to hit that. https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:351: printf("%f %f\n", total_time, average_time); On 2017/05/15 15:59:04, scroggo_chromium wrote: > Now that we do not print the average, is the expectation that the user will do > their own analysis? e.g. compute average, min, median, max? > > Should that be printed as part of this program? I want to be able to visualize the data. It also lets you see how noisy your results are and trim as necessary. An animated image may have one frame that decodes a large area and another frame which decodes a small area. So you need to treat these frames separately. So we definitely want to start dealing with more data anyway. But visualizing the data has some serious benefits so we need access to all the data points. With min we know the fastest it can go. Adding median, we can see roughly how noisy our measurements are by how much higher the median is from the average. If we weren't to visualize, those would be the values we want. But even with them you don't get the full picture. Say there is some noise -- is it a lot? Is that an expected amount of noise? What caused it? You're only option is to run it again and maybe you'll get different noise. Visualizing could help you say "whoa, right in the middle here times were very different. Now I can investigate what might be causing that." https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:8: // during image decoding on supported platforms (default off). Usage: On 2017/05/15 15:59:04, scroggo_chromium wrote: > Add comments about using --raster Done. https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:210: void Print1DResults(const std::vector<double>& timings) { On 2017/05/15 15:59:04, scroggo_chromium wrote: > I suppose you put this here to keep it near Print2DResults? My preference would > be to put it closer to where it's used. > > That said, it's used only once, and it seems pretty straightforward to just > print the iteration time where you currently add it to the vector. Done. https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:217: // The outer vector represents multiple iterations through the image. On 2017/05/15 15:59:04, scroggo_chromium wrote: > This comment appears several times. It makes me think the code itself should be > clearer. Maybe come up with a variable name that makes it more clear? I'm not > sure what that would be though. Another option: create a type that makes it more > clear, with a single comment. Some ideas: > > // The outer vector represents multiple iterations through the image. > // The inner vector represents individual frames in an animation. > using FrameTime2DVector = std::vector<std::vector<double>>; > > or > > // FrameTimeVector[i] is a single timing of decoding frame i. > using FrameTimeVector = std::vector<double>; > > // Stores FrameTimeVectors over multiple iterations. > using IterationFrameTimeVector = std::vector<FrameTimeVector>; > > I sometimes see "using" statements in sample code and think "But then I'll have > to go look up what it is when I need to use it!" But here it might be helpful, > because if I do not remember what it means I look back to the "using" statement > and see all I need to know. > > On the other hand, maybe this can just be done in line? It seems pretty > straightforward - each time you add a time to the vector, you can just print it, > and then print \n when you finish your iteration. Then you don't need to worry > about all this explaining. Yeah. I had tried to come up with a typedef / using but my solutions were uglier. Your suggestion seems pretty good. I would definitely prefer to look it up and have only one comment. https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:245: if (!frame) { On 2017/05/15 15:59:04, scroggo_chromium wrote: > As I've stated elsewhere, I don't think this can ever happen. Do you want to > check GetStatus instead? Done. https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:255: // This function mimics what actually happens in Chromium. On 2017/05/15 15:59:04, scroggo_chromium wrote: > There are different paths that result in a decode, but I think this should be > more specific about when it actually happens this way. > > Something like: > > // This function mimics deferred decoding in Chromium when not all data has been > received yet. Done. https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:262: bool all_data_received = true; On 2017/05/15 15:59:04, scroggo_chromium wrote: > This variable ends up being shadowed by a variable in the outer loop, below. > > I don't find it necessary, though. I suppose you include it to aid readability? Right, I only have this bool for readability. It could be constexpr. It mirrors what was already in the code [1]. The shadowing was not intentional. I'll rename to avoid the shadowing. If you prefer to not have a named bool I'm okay with that. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/testi... https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:286: size_t frame_count = decoder->FrameCount(); On 2017/05/15 15:59:04, scroggo_chromium wrote: > Since you're using the same decoder as above, I don't think this value will ever > be different from total_frame_count, declared above. Oh, you're right. I want to create a new decoder here. https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:309: // Decode first frame On 2017/05/15 15:59:04, scroggo_chromium wrote: > I'm guilty of this, too, but try to keep your comments focused on why you're > doing what you're doing. Reading the code, I can see that you're decoding the > first frame. That said, it's not necessarily clear to me why you only decode the > first frame. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I forgot to mention, I will separate the raster into a separate CL. I'm working on that now.
https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (left): https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:351: printf("%f %f\n", total_time, average_time); On 2017/05/19 16:47:58, cblume wrote: > On 2017/05/15 15:59:04, scroggo_chromium wrote: > > Now that we do not print the average, is the expectation that the user will do > > their own analysis? e.g. compute average, min, median, max? > > > > Should that be printed as part of this program? > > I want to be able to visualize the data. It also lets you see how noisy your > results are and trim as necessary. > > An animated image may have one frame that decodes a large area and another frame > which decodes a small area. So you need to treat these frames separately. So we > definitely want to start dealing with more data anyway. > > But visualizing the data has some serious benefits so we need access to all the > data points. With min we know the fastest it can go. Adding median, we can see > roughly how noisy our measurements are by how much higher the median is from the > average. If we weren't to visualize, those would be the values we want. But even > with them you don't get the full picture. Say there is some noise -- is it a > lot? Is that an expected amount of noise? What caused it? You're only option is > to run it again and maybe you'll get different noise. Visualizing could help you > say "whoa, right in the middle here times were very different. Now I can > investigate what might be causing that." I guess what I'm getting at is, what do you do to visualize the data? Do you just look at it? Or do you plug it into some other program? Should that program be a part of this one? Or checked in? Or referenced somewhere in this file? https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:262: bool all_data_received = true; On 2017/05/19 16:47:58, cblume wrote: > On 2017/05/15 15:59:04, scroggo_chromium wrote: > > This variable ends up being shadowed by a variable in the outer loop, below. > > > > I don't find it necessary, though. I suppose you include it to aid > readability? > > Right, I only have this bool for readability. It could be constexpr. > It mirrors what was already in the code [1]. > The shadowing was not intentional. > > I'll rename to avoid the shadowing. If you prefer to not have a named bool I'm > okay with that. > > [1] > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/testi... Ah, I prefer not having a named bool, but it makes sense to follow existing style. (The real fix may be to use an enum. Then the call site will say setData(..., AllDataReceived::Yes) or something like that.) https://codereview.chromium.org/2880953002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:253: bool apply_color_correction, applying color correction should not make a difference for the frame count, and this is only used for the frame count, right? I suggest dropping this parameter.
https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (left): https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:351: printf("%f %f\n", total_time, average_time); On 2017/05/19 21:04:15, scroggo_chromium wrote: > On 2017/05/19 16:47:58, cblume wrote: > > On 2017/05/15 15:59:04, scroggo_chromium wrote: > > > Now that we do not print the average, is the expectation that the user will > do > > > their own analysis? e.g. compute average, min, median, max? > > > > > > Should that be printed as part of this program? > > > > I want to be able to visualize the data. It also lets you see how noisy your > > results are and trim as necessary. > > > > An animated image may have one frame that decodes a large area and another > frame > > which decodes a small area. So you need to treat these frames separately. So > we > > definitely want to start dealing with more data anyway. > > > > But visualizing the data has some serious benefits so we need access to all > the > > data points. With min we know the fastest it can go. Adding median, we can see > > roughly how noisy our measurements are by how much higher the median is from > the > > average. If we weren't to visualize, those would be the values we want. But > even > > with them you don't get the full picture. Say there is some noise -- is it a > > lot? Is that an expected amount of noise? What caused it? You're only option > is > > to run it again and maybe you'll get different noise. Visualizing could help > you > > say "whoa, right in the middle here times were very different. Now I can > > investigate what might be causing that." > > I guess what I'm getting at is, what do you do to visualize the data? Do you > just look at it? Or do you plug it into some other program? Should that program > be a part of this one? Or checked in? Or referenced somewhere in this file? I'm printing out the data in a way that makes it csv-friendly. So then I import the csv into Google Sheets, where I can visualize it as a graph and copy&trim out noise. This is the downside of adding accuracy. It necessitates more data, which in turn necessitates something to process that data. In an ideal world, we would also provide the code to visualize & manipulate the data. But that's a lot of extra work which I don't think is worth the effort just yet. A happy medium would be to allow passing in multiple image inputs and save it all to a single import into Sheets. Right now, you have to import each csv separately. I looked at a few options but none of the simple ones supported this. I'll add a comment about the output being csv-formatted. https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:262: bool all_data_received = true; On 2017/05/19 21:04:15, scroggo_chromium wrote: > On 2017/05/19 16:47:58, cblume wrote: > > On 2017/05/15 15:59:04, scroggo_chromium wrote: > > > This variable ends up being shadowed by a variable in the outer loop, below. > > > > > > I don't find it necessary, though. I suppose you include it to aid > > readability? > > > > Right, I only have this bool for readability. It could be constexpr. > > It mirrors what was already in the code [1]. > > The shadowing was not intentional. > > > > I'll rename to avoid the shadowing. If you prefer to not have a named bool I'm > > okay with that. > > > > [1] > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/testi... > > Ah, I prefer not having a named bool, but it makes sense to follow existing > style. (The real fix may be to use an enum. Then the call site will say > > setData(..., AllDataReceived::Yes) > > or something like that.) I agree that I prefer the enum. I'll leave it for now to match existing style. We can change it to an enum in a separate CL. https://codereview.chromium.org/2880953002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:253: bool apply_color_correction, On 2017/05/19 21:04:16, scroggo_chromium wrote: > applying color correction should not make a difference for the frame count, and > this is only used for the frame count, right? I suggest dropping this parameter. Ah, you're right. I can pass whatever into ImageDecoder::Create() and it won't matter. Good call.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I have separated the addition of raster timing into its own CL: https://codereview.chromium.org/2893023003
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
From the description: > Add a mode to ImageDecodeBench that measures drawing > > Some changes to ImageDecoder may have different effects on the performance of > decoding versus that of drawing the decoded image. Add a mode to ImageDecodeBench that measures the latter. This has been removed from this CL, so please remove it from the description. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:14: // Each row represents suceessive frames in an animated image. successive* https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:15: // Each column represents a successive iteration of decoding the whole animated I don't think this is correct. A row shows a single time for each frame. The second row shows a single time for each frame (on the second iteration). So a column represents all the times for a single frame, right? https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:17: // This means non-animated images will show up as a column. nit: Instead of saying "as a column", I think it would be clearer to say either "as a single column" or "as one column". https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:18: // Raster timing also shows up as a column. Since you've moved raster timing to its own patch, this comment is no longer necessary here. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:215: using FrameTimings = std::vector<double>; The names help, but I think a comment here might be useful, too. Something like: // This vector represents a single iteration of one (possibly animated) // image. Each entry is a single timing of a single frame. using FrameTimings = std::vector<double>; I thought about suggesting a comment for the second one, but I think it's pretty clear once you know what a FrameTiming is. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:243: if (frame->GetStatus() != ImageFrame::kFrameComplete) { Should this print an error message? https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:286: for (size_t iteration = 0; iteration < iterations; ++iteration) { nit: I think this would be easier to follow if you changed "iteration" to "i". https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:296: decoder->ClearCacheExceptFrame(next_frame_to_decode); I thought we had agreed to remove this? Was that on a different CL? https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:300: if (all_data_received || decoder->Failed()) Should this print an error statement? https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:384: blink::TimePacketedDecode(decoder.get(), data.Get(), packet_size, This whole file looks to be inside namespace blink, so "blink::" is not necessary here.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:14: // Each row represents suceessive frames in an animated image. On 2017/05/22 13:07:40, scroggo_chromium wrote: > successive* Done. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:15: // Each column represents a successive iteration of decoding the whole animated On 2017/05/22 13:07:40, scroggo_chromium wrote: > I don't think this is correct. A row shows a single time for each frame. > The second row shows a single time for each frame (on the second iteration). So > a column represents all the times for a single frame, right? Hrmmm I think this comment is correct. With the names below, FrameTimings = std::vector<double> IterationsOfFrameTimings = std::vector<FrameTimings> And Print2DResults: for (FrameTimings iteration : timings) { for (double frame_time : iteration) { printf("%f,", frame_time); } printf("\n"); (The code currently has const std::vector<double>& in the outer loop instead of FrameTimings. I'm going to fix that to clarify.) So with the names and print, it should be printing a row of frame times before continuing to the next row for the next iteration. This was intentional: Google Sheets allows you to use =ARRAYFORMULA for columns but not rows. And you'll want to compare frame 1 with other frame 1s to trim out noise if necessary. So I want each column to represent that frame. Anyway, the other part of this I should check is if we're filling the vector in this order as well. TimeDecode() fills the inner-vector with animation frame N. So does TimePacketedDecode(). Yeah, I think this comment is correct. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:17: // This means non-animated images will show up as a column. On 2017/05/22 13:07:40, scroggo_chromium wrote: > nit: Instead of saying "as a column", I think it would be clearer to say either > "as a single column" or "as one column". Done. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:18: // Raster timing also shows up as a column. On 2017/05/22 13:07:40, scroggo_chromium wrote: > Since you've moved raster timing to its own patch, this comment is no longer > necessary here. Done. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:215: using FrameTimings = std::vector<double>; On 2017/05/22 13:07:40, scroggo_chromium wrote: > The names help, but I think a comment here might be useful, too. Something like: > > // This vector represents a single iteration of one (possibly animated) > // image. Each entry is a single timing of a single frame. > using FrameTimings = std::vector<double>; > > I thought about suggesting a comment for the second one, but I think it's pretty > clear once you know what a FrameTiming is. I agree. Your suggested comment is very clear. Thank you. Done. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:243: if (frame->GetStatus() != ImageFrame::kFrameComplete) { On 2017/05/22 13:07:40, scroggo_chromium wrote: > Should this print an error message? Done. I also am using exit(3); to mirror current behavior as closely as I can. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:286: for (size_t iteration = 0; iteration < iterations; ++iteration) { On 2017/05/22 13:07:40, scroggo_chromium wrote: > nit: I think this would be easier to follow if you changed "iteration" to "i". Done. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:296: decoder->ClearCacheExceptFrame(next_frame_to_decode); On 2017/05/22 13:07:40, scroggo_chromium wrote: > I thought we had agreed to remove this? Was that on a different CL? Right. I thought I had removed it, too. It looks like it was a different CL: https://codereview.chromium.org/2880533002 That CL is upstream of this one. So we should pick this up when I rebase in a bit. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:300: if (all_data_received || decoder->Failed()) On 2017/05/22 13:07:40, scroggo_chromium wrote: > Should this print an error statement? Done. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:384: blink::TimePacketedDecode(decoder.get(), data.Get(), packet_size, On 2017/05/22 13:07:40, scroggo_chromium wrote: > This whole file looks to be inside namespace blink, so "blink::" is not > necessary here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/22 13:07:41, scroggo_chromium wrote: > From the description: > > > Add a mode to ImageDecodeBench that measures drawing > > > > Some changes to ImageDecoder may have different effects on the performance of > > decoding versus that of drawing the decoded image. Add a mode to > ImageDecodeBench that measures the latter. > > This has been removed from this CL, so please remove it from the description. This is still listed in the description. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:15: // Each column represents a successive iteration of decoding the whole animated On 2017/05/23 00:43:56, cblume wrote: > On 2017/05/22 13:07:40, scroggo_chromium wrote: > > I don't think this is correct. A row shows a single time for each frame. > > The second row shows a single time for each frame (on the second iteration). > So > > a column represents all the times for a single frame, right? > > Hrmmm I think this comment is correct. > > With the names below, > FrameTimings = std::vector<double> > IterationsOfFrameTimings = std::vector<FrameTimings> > > And Print2DResults: > for (FrameTimings iteration : timings) { > for (double frame_time : iteration) { > printf("%f,", frame_time); > } > printf("\n"); > > (The code currently has const std::vector<double>& in the outer loop instead of > FrameTimings. I'm going to fix that to clarify.) > > So with the names and print, it should be printing a row of frame times before > continuing to the next row for the next iteration. Agreed. This is covered by the comment above. ("Each row represents suceessive frames in an animated image.") > > This was intentional: Google Sheets allows you to use =ARRAYFORMULA for columns > but not rows. And you'll want to compare frame 1 with other frame 1s to trim out > noise if necessary. In https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... I asked how you are visualizing the data. This comment answers that question, and I think would be helpful in the code. > So I want each column to represent that frame. That's my interpretation of what it does. Just to be clear, when you say "frame", you mean an |ImageFrame|, as returned by ImageDecoder::FrameBufferAtIndex, right? The comment says "Each column represents a successive iteration of decoding *the whole animated image*" (emphasis mine). "The whole animated image" does not sound like a single frame, it sounds like all of the frames. > > Anyway, the other part of this I should check is if we're filling the vector in > this order as well. TimeDecode() fills the inner-vector with animation frame N. > So does TimePacketedDecode(). > > Yeah, I think this comment is correct. My interpretation of this code review comment is that a column represents a single frame. That's not what the comment in the code sounds like to me. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:296: decoder->ClearCacheExceptFrame(next_frame_to_decode); On 2017/05/23 00:43:56, cblume wrote: > On 2017/05/22 13:07:40, scroggo_chromium wrote: > > I thought we had agreed to remove this? Was that on a different CL? > > Right. > I thought I had removed it, too. > It looks like it was a different CL: > https://codereview.chromium.org/2880533002 > That CL is upstream of this one. So we should pick this up when I rebase in a > bit. I don't think rebasing works the way you expect. This CR shows that you are adding this line. https://codereview.chromium.org/2880953002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:296: exit(3); Please pass different values to "exit" so it can easily be determined which call to exit resulted in the program exiting.
https://codereview.chromium.org/2880953002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:296: exit(3); On 2017/05/23 15:42:23, scroggo_chromium wrote: > Please pass different values to "exit" so it can easily be determined which call > to exit resulted in the program exiting. My mistake, it looks like exit is already not using unique values.
Description was changed from ========== ImageDecodeBench has a few places where it allows noise to dominate measurements: * It runs all iterations and divides by iteration count to get the average. This takes long enough that the process is swapped out by the OS. This adds noise. * It includes setup and teardown in the time, * It runs through all frames of an animated image and again averages. If an animated image must decode a large first frame but all subsequent frames are small sub-region updates, the average will be faster than the slowest decode and slower than the faster decodes. It will not represent what actually happened. For example, if decode times are improved that will be buried under the small frame times that don't spend much time in decode. To more accurately represent measurements, start the timer immediately before decoding a single frame, decode the frame, and then immediately stop the timer. Report these times separately for each frame. This addresses the above problems: * it removes setup and teardown from the measurement * it lessens any impact of OS swapping processes * times are not skewed by differently sized frames Add a mode to ImageDecodeBench that measures drawing Some changes to ImageDecoder may have different effects on the performance of decoding versus that of drawing the decoded image. Add a mode to ImageDecodeBench that measures the latter. BUG=722147 ========== to ========== ImageDecodeBench has a few places where it allows noise to dominate measurements: * It runs all iterations and divides by iteration count to get the average. This takes long enough that the process is swapped out by the OS. This adds noise. * It includes setup and teardown in the time, * It runs through all frames of an animated image and again averages. If an animated image must decode a large first frame but all subsequent frames are small sub-region updates, the average will be faster than the slowest decode and slower than the faster decodes. It will not represent what actually happened. For example, if decode times are improved that will be buried under the small frame times that don't spend much time in decode. To more accurately represent measurements, start the timer immediately before decoding a single frame, decode the frame, and then immediately stop the timer. Report these times separately for each frame. This addresses the above problems: * it removes setup and teardown from the measurement * it lessens any impact of OS swapping processes * times are not skewed by differently sized frames BUG=722147 ==========
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/22 13:07:41, scroggo_chromium wrote: > This is still listed in the description. Removed. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:15: // Each column represents a successive iteration of decoding the whole animated On 2017/05/23 15:42:23, scroggo_chromium wrote: > In > https://codereview.chromium.org/2880953002/diff/1/third_party/WebKit/Source/p... > I asked how you are visualizing the data. This comment answers that question, > and I think would be helpful in the code. That is probably a good idea. I'll add comments after the "this is csv formatted" to help the user understand how to then use the data. > That's my interpretation of what it does. Just to be clear, when you say > "frame", you mean an |ImageFrame|, as returned by > ImageDecoder::FrameBufferAtIndex, right? The comment says "Each column > represents a successive iteration of decoding *the whole animated image*" > (emphasis mine). "The whole animated image" does not sound like a single frame, > it sounds like all of the frames. Yes, I mean an individual frame. Hrmmm I guess this is not very clear, huh? What I mean is column 0 represents frame 0. Column 1 represents frame 1. Each entry in the column is a separate timing. And the timing happens after all the other animation frames have been decoded. So (0,0) is the first frame and first iteration. This code will decode the rest of the animation frames before decoding (0,1) which is the first frame of the second iteration. That is why I mentioned the whole image. But I think mentioning that makes things less clear. I'll change it to: "Each column represents a single frame decoded from successive iterations." In fact, I think I want to add examples. "Each row represents successive frames in an animated image. (frame 1, frame 2, frame 3...) Each column represents a single frame decoded from successive iterations. Iteration 1: (frame 1, frame 2, frame 3...) Iteration 2: (frame 1, frame 2, frame 3...)" > > > > > Anyway, the other part of this I should check is if we're filling the vector > in > > this order as well. TimeDecode() fills the inner-vector with animation frame > N. > > So does TimePacketedDecode(). > > > > Yeah, I think this comment is correct. > > My interpretation of this code review comment is that a column represents a > single frame. That's not what the comment in the code sounds like to me. https://codereview.chromium.org/2880953002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:296: decoder->ClearCacheExceptFrame(next_frame_to_decode); On 2017/05/23 15:42:23, scroggo_chromium wrote: > On 2017/05/23 00:43:56, cblume wrote: > > On 2017/05/22 13:07:40, scroggo_chromium wrote: > > > I thought we had agreed to remove this? Was that on a different CL? > > > > Right. > > I thought I had removed it, too. > > It looks like it was a different CL: > > https://codereview.chromium.org/2880533002 > > That CL is upstream of this one. So we should pick this up when I rebase in a > > bit. > > I don't think rebasing works the way you expect. This CR shows that you are > adding this line. Right. I hadn't rebased by the time you saw this line here. But I also didn't check *after* rebasing and it is still there. It looks like the auto-rebase didn't pick it up since things had been moved around. I'll remove it. Thanks for spotting this.
https://codereview.chromium.org/2880953002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:16: // Each column represents a successive iteration of decoding the whole animated You said you would change this to "Each column represents a single frame decoded from successive iterations." Is that in a forth-coming patch? https://codereview.chromium.org/2880953002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:18: // Iteration 1: (frame 1, frame 2, frame 3...) nit: I think in zero-based indices. https://codereview.chromium.org/2880953002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:24: // imported into Google Sheets. From Sheets, the user can create graphs to Sorry, I should've been more clear here. We probably don't want to mention/recommend a Google product. (I'm guessing using some other spreadsheet program would have similar benefits?) I was thinking more like specify the types of graph to create, etc. If you think that's too much detail to provide here, that's fine with me. (And the commands probably are specific.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2880953002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:16: // Each column represents a successive iteration of decoding the whole animated On 2017/05/23 21:04:23, scroggo_chromium wrote: > You said you would change this to "Each column represents a single frame decoded > from successive iterations." Is that in a forth-coming patch? Oops. Done. https://codereview.chromium.org/2880953002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:18: // Iteration 1: (frame 1, frame 2, frame 3...) On 2017/05/23 21:04:23, scroggo_chromium wrote: > nit: I think in zero-based indices. Done. https://codereview.chromium.org/2880953002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:24: // imported into Google Sheets. From Sheets, the user can create graphs to On 2017/05/23 21:04:23, scroggo_chromium wrote: > Sorry, I should've been more clear here. We probably don't want to > mention/recommend a Google product. (I'm guessing using some other spreadsheet > program would have similar benefits?) > > I was thinking more like specify the types of graph to create, etc. If you think > that's too much detail to provide here, that's fine with me. (And the commands > probably are specific.) Oh, you're right. I definitely don't want Chromium to feel like it excludes non-Google solutions. I've changed the wording around in a way that I think might be more clear.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2880953002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:265: timings[i][frame_index] = elapsed_time; I was measuring decode times just now so we can wrap up https://codereview.chromium.org/2756463003/# I noticed something though. The ImageDecoder's cache is working and the decode times drop quickly as we loop. In the morning I'll change this so we create a new image decoder each iteration, rather than pass in the ImageDecoder.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/24 10:04:16, cblume wrote: > I noticed something though. The ImageDecoder's cache is working and the decode > times drop quickly as we loop. There was another piece I noticed when reusing the existing ImageDecoder: It seemed to only mark the final frame as Complete. As a result, the next iteration through the animated image would decode all the frames but the last again. Here is an example of the measurements (requires @chromium.org account): https://docs.google.com/a/chromium.org/spreadsheets/d/1wBWGmSvWF1gnRMUzW3A8od... This is a PERFECT example of why you want to visualize the data. Suddenly this sort of bug jumps out at you. I was able to confirm that on the second iteration, all frames but the last were still not marked as Complete. I'm going to investigate & fix this after my sheriff rotation ends.
Ping I think some of the questions over on the "Add raster timing to ImageDecodeBench" CR [1] might apply here, too. I'm addressing them over there since they were asked over there. [1] https://codereview.chromium.org/2893023003/
Ping We mentioned reporting the mean and variance. And the raw data would use a separate flag and output to a file. That is done. However, reporting the variance alone wasn't very intuitive/useful. The variance is in seconds squared and the mean is in seconds. Since our measurements are typically <1, the squared difference from mean would make VERY small numbers. IE a decode time of 0.000259 and a mean of 0.000065 gives a difference from mean of 0.000194. Squaring that gives 0.000000037636. If we were to display a number like that it would show up as 0.000000. But even if we displayed the whole number, it is unwieldy to work in seconds & seconds squared. That variance looks very small and makes me feel confident that there isn't much noise: mean: 0.000065 variance: 0.000000 But comparing seconds to seconds makes things much more intuitive: mean: 0.000065 standard deviation: 0.000026 By using standard deviation instead of variance (just square rooting the variance), I am able to compare apples to apples and better understand my noise.
Ping x3 To clarify, standard deviation and variance represent the same thing. I'm just using comparable units (seconds to seconds). But you can square the standard deviation to get the variance back.
https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:13: // If --raw-output is specified, the output is formatted for use in a .csv file - The description seems to imply that this is the default. - "Otherwise, the output is..." - This statement is confusing. In truth, the output appears to be a single line with the mean and the standard deviation. --raw-output allows you to specify a file to which you write the following data. https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:262: double GetMean(const IterationsOfFrameTimings& timings) { The description explains why we shouldn't average all the frame iterations together, but here we're doing it anyway. https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:378: if (all_data_received || decoder->Failed()) { Wait, why do we print failure and exit if all_data_received? https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:419: "Usage: %s [--color-correct] [--raw-output output_file] file " Maybe "file" should now be "input_file", or "image_file"?
Description was changed from ========== ImageDecodeBench has a few places where it allows noise to dominate measurements: * It runs all iterations and divides by iteration count to get the average. This takes long enough that the process is swapped out by the OS. This adds noise. * It includes setup and teardown in the time, * It runs through all frames of an animated image and again averages. If an animated image must decode a large first frame but all subsequent frames are small sub-region updates, the average will be faster than the slowest decode and slower than the faster decodes. It will not represent what actually happened. For example, if decode times are improved that will be buried under the small frame times that don't spend much time in decode. To more accurately represent measurements, start the timer immediately before decoding a single frame, decode the frame, and then immediately stop the timer. Report these times separately for each frame. This addresses the above problems: * it removes setup and teardown from the measurement * it lessens any impact of OS swapping processes * times are not skewed by differently sized frames BUG=722147 ========== to ========== ImageDecodeBench currently reports the mean without reporting standard deviation / variance. This means we cannot perfectly compare before & after measurements -- we need to make sure the central limit theorem has enough samples to maintain consistent standard deviation / variance between runs. ImageDecodeBench will now report the standard deviation along with the mean so the user can verify that the before & after runs have a consistent amount of noise. In order to assist the standard deviation / variance from changing much between runs, we can also increase the precision of our timing. This patch does that by starting the timer immediately before decoding a single frame, decoding the frame, and then immediately stopping the timer. It also no longer includes setup and teardown in the measurement. BUG=722147 ==========
Description was changed from ========== ImageDecodeBench currently reports the mean without reporting standard deviation / variance. This means we cannot perfectly compare before & after measurements -- we need to make sure the central limit theorem has enough samples to maintain consistent standard deviation / variance between runs. ImageDecodeBench will now report the standard deviation along with the mean so the user can verify that the before & after runs have a consistent amount of noise. In order to assist the standard deviation / variance from changing much between runs, we can also increase the precision of our timing. This patch does that by starting the timer immediately before decoding a single frame, decoding the frame, and then immediately stopping the timer. It also no longer includes setup and teardown in the measurement. BUG=722147 ========== to ========== ImageDecodeBench currently reports the mean without reporting standard deviation / variance. This means we cannot perfectly compare before & after measurements -- we need to make sure the central limit theorem has enough samples to maintain consistent standard deviation / variance between runs. ImageDecodeBench will now report the standard deviation along with the mean so the user can verify that the before & after runs have a consistent amount of noise. In order to assist the standard deviation / variance from changing much between runs, we can also increase the precision of our timing. This patch does that by starting the timer immediately before decoding a single frame, decoding the frame, and then immediately stopping the timer. It also no longer includes setup and teardown in the measurement. Since we are now timing individual runs to find the standard deviation / variance, we also added a --raw-output command line option which saves the measurements in a csv-formatted file. This can then be used in external visualization tools. BUG=722147 ==========
noel@ I would like to get your thoughts as well. https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:13: // If --raw-output is specified, the output is formatted for use in a .csv file On 2017/06/13 20:55:57, scroggo_chromium wrote: > - The description seems to imply that this is the default. > - "Otherwise, the output is..." > - This statement is confusing. In truth, the output appears to be a single line > with the mean and the standard deviation. --raw-output allows you to specify a > file to which you write the following data. You are right. I hadn't updated the description after this change was made. I've updated the description and changed this comment to make a more clear distinction between the console output and the file output. https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:262: double GetMean(const IterationsOfFrameTimings& timings) { On 2017/06/13 20:55:57, scroggo_chromium wrote: > The description explains why we shouldn't average all the frame iterations > together, but here we're doing it anyway. Good point. After we talked about the central limit theorem handling noise I am less worried about the process getting swapped out and us not seeing it. Especially since we added output to tell us our variance / standard deviation. I spoke to a statistician here at Google (gusl@) and asked for his advice about what we should track and how. He suggested the ideal thing to track would be the minimum of a given run. This is because all possible noise is additive. The minimum should be the noise-free, apples-to-apples ground truth that we can compare. But he also said the mean + standard deviation will mirror the minimum if we make sure our standard deviation is the same before & after. We shouldn't be introducing noise between runs so this should be fine. (I've timed this before & after this patch to make sure we didn't change the standard deviation / variance. I'll send that out later.) Prior to this patch (without reporting the standard deviation / variance), using the mean alone was actually a problem. Imagine a situation where we have made an optimization so the minimum has decreased. However, when we measured we accidentally increased our variance and effectively kept the mean at the same place. We wouldn't have known that we didn't run enough iterations for the central limit theorem to overcome that noise. I'll update the description. https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:378: if (all_data_received || decoder->Failed()) { On 2017/06/13 20:55:57, scroggo_chromium wrote: > Wait, why do we print failure and exit if all_data_received? Good question. all_data_received should not be here. The current ImageDecodeBench has this line. But rather than printing a failure and exiting, it uses it to break from the infinite loop. This is how it knows we have no more packets to test with. It was my mistake to keep it as-is. https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:419: "Usage: %s [--color-correct] [--raw-output output_file] file " On 2017/06/13 20:55:57, scroggo_chromium wrote: > Maybe "file" should now be "input_file", or "image_file"? Done.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please add some line breaks to your commit message. The convention is to use a single line summary that is 50 characters, followed by a blank line, followed by the rest of your description (which has line breaks after at most 72 characters). Some more specific comments on your commit message follow: > ImageDecodeBench currently reports the mean without reporting standard deviation / variance. This means we cannot perfectly compare before & after measurements -- we need to make sure the central limit theorem has enough samples to maintain consistent standard deviation / variance between runs. Just to reiterate, you're missing a 50 character summary line. I think this is a fine paragraph to start your longer description, though. > ImageDecodeBench will now report the standard deviation along with the mean so the user can verify that the before & after runs have a consistent amount of noise. nit: Use the imperative tone. e.g. Make ImageDecodeBench report the standard deviation along with the mean so the user can verify that two runs have a consistent amount of noise. (I also recommend changing "before & after" to "separate". "Before & after" makes me want to know "before and after what?" It takes another step to imagine you're talking about a hypothetical change you want to make. Really you just want runs to be generally consistent.) > In order to assist the standard deviation / variance from changing much between runs, we can also increase the precision of our timing. This patch does that by starting the timer immediately before decoding a single frame, decoding the frame, and then immediately stopping the timer. It also no longer includes setup and teardown in the measurement. Again, imperative is better, and I think you can shorten/simplify this paragraph without losing any meaning: Increase the precision of timing by starting the timer immediately before decoding a frame and stopping it immediately after. This removes setup and teardown from the measurements. > Since we are now timing individual runs to find the standard deviation / variance, we also added a --raw-output command line option which saves the measurements in a csv-formatted file. This can then be used in external visualization tools. Again, I think this can be simpler (and imperative): Add the --raw-output command line option which saves individual measurements to the file provided in csv-format. This can then be used in external visualization tools. https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:262: double GetMean(const IterationsOfFrameTimings& timings) { On 2017/06/14 21:18:37, cblume wrote: > On 2017/06/13 20:55:57, scroggo_chromium wrote: > > The description explains why we shouldn't average all the frame iterations > > together, but here we're doing it anyway. > > Good point. > After we talked about the central limit theorem handling noise I am less worried > about the process getting swapped out and us not seeing it. Especially since we > added output to tell us our variance / standard deviation. > > I spoke to a statistician here at Google (gusl@) and asked for his advice about > what we should track and how. He suggested the ideal thing to track would be the > minimum of a given run. This is because all possible noise is additive. The > minimum should be the noise-free, apples-to-apples ground truth that we can > compare. > > But he also said the mean + standard deviation will mirror the minimum if we > make sure our standard deviation is the same before & after. We shouldn't be > introducing noise between runs so this should be fine. (I've timed this before & > after this patch to make sure we didn't change the standard deviation / > variance. I'll send that out later.) > > Prior to this patch (without reporting the standard deviation / variance), using > the mean alone was actually a problem. Imagine a situation where we have made an > optimization so the minimum has decreased. However, when we measured we > accidentally increased our variance and effectively kept the mean at the same > place. We wouldn't have known that we didn't run enough iterations for the > central limit theorem to overcome that noise. > > I'll update the description. Should we report the min, too? https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:334: ImageDecoder::Create(data.Get(), true, ImageDecoder::kAlphaPremultiplied, Why not move "total_all_data_received" up here and use it here instead of "true"? https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:338: frame_count_decoder->SetData(data.Get(), total_all_data_received); This line is unnecessary. Create has already set the data. https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:339: size_t total_frame_count = frame_count_decoder->FrameCount(); The start of this method looks like it could use a refactor into its own method: size_t GetFrameCount(PassRefPtr<SharedBuffer> data) { auto decoder = ImageDecoder::Create(...); if (!decoder) { fprintf(stderr, "Image decode failed\n"); exit(3); } size_t frame_count = decoder->FrameCount(); if (decoder->Failed()) { fprintf(stderr, "Image decode failed\n"); exit(3); } return frame_count; } (I also added some failure cases. You currently catch the Failed() piece later. Without the null check, we'd see a crash if you passed a file that is not an image file we can decode. Maybe it's fine to leave that one out especially (user error.)) https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:344: RefPtr<SharedBuffer> packet_data = SharedBuffer::Create(); I think these three variables need to be inside the iterations loop. Otherwise, you'll end up with a SharedBuffer that has the image repeated over and over again. (And all_data_received will be false for all iterations beyond the first, since position > data->size().) https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:360: data.Get(), true, ImageDecoder::kAlphaPremultiplied, The second parameter here is whether all the data has been received. So you probably don't want true here, you want all_data_received. Stepping back, though, what are we timing here? I *think* the goal is to create a decoder, and then pass the data in packets, but that doesn't look to be what's happening here. Instead, you're creating a new decoder with each packet. You've moved the iterations loop inside this method, but it should still have the same structure as the old way of doing it: foreach iteration: decoder = ImageDecoder::Create() SharedBufffer packet_data; while true: packet_data->append(packet_size) DecodeTheFramesWeCan https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:371: fprintf(stderr, "Image decode failed\n"); In the old code, this was a "break" - we haven't decoded a full frame, so we wait until we have more data. Why did you make this a failure case? https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:377: if (all_data_received || decoder->Failed()) { In patch set 16 you said all_data_received should be removed from this line. With it here, it seems like we would always hit this case eventually.
On 2017/06/14 21:18:37, cblume wrote: > noel@ I would like to get your thoughts as well. > > https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): > > https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:13: // If > --raw-output is specified, the output is formatted for use in a .csv file > On 2017/06/13 20:55:57, scroggo_chromium wrote: > > - The description seems to imply that this is the default. > > - "Otherwise, the output is..." > > - This statement is confusing. In truth, the output appears to be a single > line > > with the mean and the standard deviation. --raw-output allows you to specify a > > file to which you write the following data. > > You are right. I hadn't updated the description after this change was made. > > I've updated the description and changed this comment to make a more clear > distinction between the console output and the file output. > > https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:262: double > GetMean(const IterationsOfFrameTimings& timings) { > On 2017/06/13 20:55:57, scroggo_chromium wrote: > > The description explains why we shouldn't average all the frame iterations > > together, but here we're doing it anyway. > > Good point. > After we talked about the central limit theorem handling noise I am less worried > about the process getting swapped out and us not seeing it. Especially since we > added output to tell us our variance / standard deviation. and use a controlled environment while you measure. Screen saver, power management, all applications closed, and other factors I mentioned in the central limit discussion [1]. If you're surfing the web on machine while you are awaiting benchmark results from that machine, then your results will be dubious. Your machine should not be used while you are measuring. [1] https://codereview.chromium.org/2893023003/#msg48 > I spoke to a statistician here at Google (gusl@) and asked for his advice about > what we should track and how. He suggested the ideal thing to track would be the > minimum of a given run. This is because all possible noise is additive. The > minimum should be the noise-free, apples-to-apples ground truth that we can > compare. Minimum is fine, but remember it is still a data point from a sample of data, and that data has the same statistic, or statistic distribution. > But he also said the mean + standard deviation will mirror the minimum if we > make sure our standard deviation is the same before & after. We shouldn't be > introducing noise between runs so this should be fine. (I've timed this before & > after this patch to make sure we didn't change the standard deviation / > variance. I'll send that out later.) Yes. The statistical distribution has a variance, and should be the same before and after. > Prior to this patch (without reporting the standard deviation / variance), using > the mean alone was actually a problem. No, it wasn't problem. > Imagine a situation where we have made an > optimization so the minimum has decreased. However, when we measured we > accidentally increased our variance and effectively kept the mean at the same > place. We wouldn't have known that we didn't run enough iterations for the > central limit theorem to overcome that noise. Supposition: the variance increased you say -> different controlled environment, and you would not be comparing the before and after effects of the proposed change to image decoding. Your results would reflect some change in the environment, not the win/loss of the image decoding change. You might only conclude that surfing the web while measuring altered your measurements :)
https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:371: fprintf(stderr, "Image decode failed\n"); On 2017/06/15 14:03:51, scroggo_chromium wrote: > In the old code, this was a "break" - we haven't decoded a full frame, so we > wait until we have more data. Why did you make this a failure case? Good spot. A second question is: why are we measuring the time of each frame of multi-frame image anyway? What does it buy us? Some frames of an animated image will cost X time to decode, some will cost X/10 to decode because they are a delta. Most delta frames are relatively fast to decode, which is the whole point of animated image formats. Can't help thinking that all we want to know is how long does it take to decode all frames of the given image. And the code would be a hell of a lot simpler if we did that (see code left).
On 2017/06/16 17:32:56, Noel Gordon wrote: > Why are we measuring the time of each frame of > multi-frame image anyway? What does it buy us? > > Some frames of an animated image will cost X time to decode, some will cost X/10 > to decode because they are a delta. Most delta frames are relatively fast to > decode, which is the whole point of animated image formats. Exactly!! :D If an image has 100 frames, but only frame 0 is slow, the average will mostly represent the fast frames, which spend relatively little time actually decoding. Using the pre-patch code, jpegs, non-animated pngs, and non-animated gifs all have a low standard deviation. However, several animated gifs have a standard deviation that is over 30% of the mean.* Since I am mostly working on gifs right now, this is my motivation. This is also what I had mentioned in the bug and in the original CR description with: "* It runs through all frames of an animated image and again averages. If an animated image must decode a large first frame but all subsequent frames are small sub-region updates," On 2017/06/16 17:32:56, Noel Gordon wrote: > Can't help thinking that all we want to know is how long does it take to decode > all frames of the given image. And the code would be a hell of a lot simpler if > we did that (see code left). It still does that. That hasn't changed. It just no longer includes non-decoding in the timing. It is strictly a more accurate measurement. *Although, something that is different is it is now taking the average of frames rather than the average of iterations. I could add up all the frames of an iteration and take the average of that. This would be closer to what currently happens and would bring the standard deviation back down. Either way, it would be converging to the same mean. But since the decode time of frame 0 is inherently unrelated to the decode time of frame 1, putting them in the same average isn't particularly useful either. You could also decode all frames of all gifs and count that as 1 iteration. Gif A is unrelated to gif B. So I'm not sure adding the frame times and averaging iterations is what we really want.** I'm more interested in comparing frame 0s together, then comparing frame 1s together. That way if I improve decode time, it is more likely to show up in the measurements. Said another way, it is less likely to get buried in the non-decoding overhead done for small delta updates. **Another option which brings the standard deviation down is to only report on frame 0, which all images will have and is most likely to be a full frame decode. I'm in favor of this. I think it solves all of our needs. And it is most likely to show actual decoding in the measurements, which is the whole namesake of image_decode_bench. On 2017/06/16 17:31:55, Noel Gordon wrote: > and use a controlled environment while you measure. Screen saver, power > management, all applications closed, and other factors I mentioned in the > central limit discussion [1]. If you're surfing the web on machine while you > are awaiting benchmark results from that machine, then your results will be > dubious. Your machine should not be used while you are measuring. > > [1] https://codereview.chromium.org/2893023003/#msg48 I had done this. I rebooted my machine the night before so any startup processes could hopefully finish. I opened only a terminal. No screen saver. The next morning, I began running tests. This is when I was getting >30% standard deviation with the pre-patch code on animated gifs. This is why I'm writing this patch in the first place. I could do better by disconnecting the ethernet cable and any non-essential USB devices. But I don't expect to see a difference if I were to do that. I did not check power management on my desktop. That could play a large factor but I would be surprised if our beefy desktops are set to anything other than performance. I also ran the test many times with increasing iterations to make sure we were converging. All my tests had a minimum of 2x the iterations of test-gif.sh. It seemed to have converged as well as it could. On 2017/06/16 17:32:56, Noel Gordon wrote: > > Prior to this patch (without reporting the standard deviation / variance), using > > the mean alone was actually a problem. > No, it wasn't problem. I think we might be talking along different lines. So long as the variance didn't change between runs, there is no problem. And we can go to extremes to reduce the likelihood of the variance changing between runs. But without reporting the variance, we wouldn't know. That is a problem.
Oops, quick correction: On 2017/06/28 18:51:08, cblume wrote: > Either way, it would be converging to the same mean. They both converge to *a* mean, but not the *same* mean. If I were to sum all the frames of an animation, and take the average of that over many iterations, it would be what the pre-patch code does and would converge to the same mean.
https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:262: double GetMean(const IterationsOfFrameTimings& timings) { On 2017/06/15 14:03:50, scroggo_chromium wrote: > Should we report the min, too? I say yes. I just didn't want to rock the boat too much -- small steps are easier to accept. :) https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:334: ImageDecoder::Create(data.Get(), true, ImageDecoder::kAlphaPremultiplied, On 2017/06/15 14:03:50, scroggo_chromium wrote: > Why not move "total_all_data_received" up here and use it here instead of > "true"? Done. https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:338: frame_count_decoder->SetData(data.Get(), total_all_data_received); On 2017/06/15 14:03:51, scroggo_chromium wrote: > This line is unnecessary. Create has already set the data. Done. https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:339: size_t total_frame_count = frame_count_decoder->FrameCount(); On 2017/06/15 14:03:50, scroggo_chromium-OOO wrote: > The start of this method looks like it could use a refactor into its own method: > > size_t GetFrameCount(PassRefPtr<SharedBuffer> data) { > auto decoder = ImageDecoder::Create(...); > if (!decoder) { > fprintf(stderr, "Image decode failed\n"); > exit(3); > } > size_t frame_count = decoder->FrameCount(); > if (decoder->Failed()) { > fprintf(stderr, "Image decode failed\n"); > exit(3); > } > return frame_count; > } > > (I also added some failure cases. You currently catch the Failed() piece later. > Without the null check, we'd see a crash if you passed a file that is not an > image file we can decode. Maybe it's fine to leave that one out especially (user > error.)) Done. https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:344: RefPtr<SharedBuffer> packet_data = SharedBuffer::Create(); On 2017/06/15 14:03:51, scroggo_chromium-OOO wrote: > I think these three variables need to be inside the iterations loop. Otherwise, > you'll end up with a SharedBuffer that has the image repeated over and over > again. (And all_data_received will be false for all iterations beyond the first, > since position > data->size().) Done. https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:360: data.Get(), true, ImageDecoder::kAlphaPremultiplied, On 2017/06/15 14:03:51, scroggo_chromium-OOO wrote: > The second parameter here is whether all the data has been received. So you > probably don't want true here, you want all_data_received. > > Stepping back, though, what are we timing here? I *think* the goal is to create > a decoder, and then pass the data in packets, but that doesn't look to be what's > happening here. Instead, you're creating a new decoder with each packet. > > You've moved the iterations loop inside this method, but it should still have > the same structure as the old way of doing it: > > foreach iteration: > decoder = ImageDecoder::Create() > SharedBufffer packet_data; > while true: > packet_data->append(packet_size) > DecodeTheFramesWeCan Good catch. I'm embarrassed. :) Thank you for seeing this. https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:371: fprintf(stderr, "Image decode failed\n"); On 2017/06/16 17:32:56, Noel Gordon wrote: > On 2017/06/15 14:03:51, scroggo_chromium wrote: > > In the old code, this was a "break" - we haven't decoded a full frame, so we > > wait until we have more data. Why did you make this a failure case? Whoops. This shouldn't be a failure case. When we added early exits in patch set 8 I accidentally added this. Fixed. https://codereview.chromium.org/2880953002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:377: if (all_data_received || decoder->Failed()) { On 2017/06/15 14:03:50, scroggo_chromium wrote: > In patch set 16 you said all_data_received should be removed from this line. > With it here, it seems like we would always hit this case eventually. Done.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please reformat the commit message to have more line breaks. It's hard to read when it is so wide. https://codereview.chromium.org/2880953002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:361: RefPtr<SharedBuffer> packet_data = SharedBuffer::Create(); You probably want at least one packet before you attempt to create the ImageDecoder. I assume decoder will always be nullptr here.
https://codereview.chromium.org/2880953002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:271: double min = 0.0; This can't be right. The minimum reported by this function will always be 0.0 since the cost of an image decode is greater than 0.0 always.
https://codereview.chromium.org/2880953002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:271: double min = 0.0; On 2017/07/17 02:17:08, Noel Gordon wrote: > This can't be right. The minimum reported by this function will always be 0.0 > since the cost of an image decode is greater than 0.0 always. Done.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2880953002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp (right): https://codereview.chromium.org/2880953002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:44: #include <vector> Use WTF::Vector https://codereview.chromium.org/2880953002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:231: // This vector represents a single iteration of one (possibly animated) image. "This"? Which vector are u referring to? https://codereview.chromium.org/2880953002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:261: class MeanAndMin { Could we make this a struct please. https://codereview.chromium.org/2880953002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:292: double difference = frame_time - mean; 1) { double difference = frame_time - mean; squared_difference_sum += difference * difference; ++count; } here perhaps, but ... 2) can you explain why having a single _mean_ for some number of decode bench iterations of a multi-framed image makes sense? https://codereview.chromium.org/2880953002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:303: size_t GetFrameCount(const PassRefPtr<SharedBuffer>& data) { PassRefPtr ? Maybe pass by raw ptr. https://codereview.chromium.org/2880953002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp:343: } Should we be testing for decoder->Failed() around here? |
