performance tests for ImageDecoder |
|||
Issue descriptionWe already have a benchmark: ImageDecodeBench [1], but it has bit-rotted, due to the fact that it is not built on any bots. We need to do the following: - get the benchmark building again - make it get built in the waterfall so it continues to build (I'm new to Chromium's performance tests; where should this go? [2] recommends using Telemetry, but this is different, since it does not use a webpage/browser. Does Blink have its own performance benchmarks elsewhere?) - support it with gn (I had to use gyp to get it to build) Follow-up steps: - Actually run it on the waterfall? Which files? It looks like there is a high bar for automation (from [3]: "Whenever possible, avoid creating new benchmarks"), but I think it's reasonable to at least keep it from bit-rotting so developers can use it locally. Noel and I have used it a lot, and others have expressed interest in a benchmark that just times image decodes. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/web/ImageDecodeBench.cpp&q=imagedecodebench&sq=package:chromium&l=1 [2] http://www.chromium.org/developers/testing/adding-performance-tests [3] https://docs.google.com/document/d/1bBKyYCW3VlUUPDpQE4xvrMFdA6tovQMZoqO9KCcmqqQ/preview
,
Apr 11 2016
Had. See https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/web/web.gyp That may have been a superset of what was needed (since I was also modifying it to time deferred decoding), but those changes (at the time) made it work again. Today, it won't build at least because of ImageDecoder::qcmsOutputDeviceProfile (which is called in ImageDecodeBench, but has been removed from ImageDecoder). I haven't dug further than that, though. I believe over email you said we need to bring that method back?
,
Apr 12 2016
Uploaded https://codereview.chromium.org/1879133002/ for review. I commented out ImageDecoder::qcmsOutputDeviceProfile(), which no longer exists on ImageDecoder. I suppose it won't work properly when testing for color profiles, but if not (e.g. if ! USE(QCMSLIB)) it should work as expected.
,
Apr 12 2016
Once we get to it, I think it would be benefitial to have a separate test for different types of images. For example, within the gif decoder we could have tests for: still, animated w/ only references to previous frame, animated w/ references to distant frames, animated w/ only global palette, animated w/ separate palettes
,
Apr 13 2016
>Today, it won't build at least because of ImageDecoder::qcmsOutputDeviceProfile (which is called in ImageDecodeBench, but has been removed from ImageDecoder). I haven't dug further than that, though. I believe over email you said we need to bring that method back? Yeap, we should bring it back.
,
May 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cab42628d6fe53b18f9d119d3999746c79b33ba5 commit cab42628d6fe53b18f9d119d3999746c79b33ba5 Author: noel <noel@chromium.org> Date: Thu May 05 16:59:48 2016 Make ImageDecodeBench build again Remove unnecessary windows #error re LEAN_AND_MEAN, including <mmsystem.h> is enough. Use size_t instead of position for SharedBuffer::getSomeData. Call base::CommandLine::Init() to avoid a SEGFAULT at start-up when Platform heap initializes and trys to read CommmandLine. Inherit from blink::Platform, not TestingPlatformSupport. Remove missing function from ImageDecoder. Instead, decode once before the bench loop to initialize Platform statics. Build: move the gyp into blink_platform_tests.gyp and the .cpp to platform/testing to allow the call to base::CommandLine per the rules in platform/testing/DEPS. This patch is based on previous patch by scroggo@chromium.org, refer to https://codereview.chromium.org/1879133002 TBR=jochen@chromium.org BUG=601198 Review-Url: https://codereview.chromium.org/1954673002 Cr-Commit-Position: refs/heads/master@{#391831} [modify] https://crrev.com/cab42628d6fe53b18f9d119d3999746c79b33ba5/third_party/WebKit/Source/platform/blink_platform_tests.gyp [rename] https://crrev.com/cab42628d6fe53b18f9d119d3999746c79b33ba5/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp [modify] https://crrev.com/cab42628d6fe53b18f9d119d3999746c79b33ba5/third_party/WebKit/Source/web/web.gyp
,
Jul 14 2016
,
Dec 5 2016
Anything more that needs to happen here?
,
Dec 5 2016
https://codereview.chromium.org/2483243003/, which recently landed, fights bit-rot and builds with GN. Next step is to make sure we build it so that it will not bit-rot further.
,
Feb 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b8644b02a35599e95cf14c05a9e71f02cfb6350e commit b8644b02a35599e95cf14c05a9e71f02cfb6350e Author: Noel Gordon <noel@chromium.org> Date: Mon Feb 12 21:48:12 2018 [image_decode_bench] Remove color-correction option Remove --color-correction option now that color correction is done by Skia (which has its own color correction performance tests). Other cleanup: FrameCount returns size_t, use |index| for frame index access consistently whether packet mode, or not. Rename iterations to decode_iterations. Remove blink:: from Platform because we're already in the blink namespace. total_time is a double so casting the divisor when computing the average decode time is not needed (due to C++ type promotion rules). Rename Main to ImageDecodeBenchMain. Bug: 601198 Change-Id: I2651771e470664f78d54fb78522a98a329d25ed9 Reviewed-on: https://chromium-review.googlesource.com/913028 Reviewed-by: Leon Scroggins <scroggo@chromium.org> Commit-Queue: Noel Gordon <noel@chromium.org> Cr-Commit-Position: refs/heads/master@{#536202} [modify] https://crrev.com/b8644b02a35599e95cf14c05a9e71f02cfb6350e/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp
,
Feb 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0342258175d72113ae72c0ce56d79a3784ad2dd0 commit 0342258175d72113ae72c0ce56d79a3784ad2dd0 Author: Noel Gordon <noel@chromium.org> Date: Tue Feb 13 00:46:55 2018 [image_decode_bench] Change BUILD gn to build an executable Currently built as a test fixture, which makes it painful to use when testing on Android: a special script is needed for Android [1], which makes Android a special snowflake when image benching. Simpler worlds can be had even for Android: build/create image decode bench binary executable. Android developers can 'adb push' the binary to their Android test device, and run it from adb shell (and no fancy scripts required). For everyone else (mac/linux...), nothing changes, just run out/Release/image_decode_bench as usual. [1] https://crbug.com/762564#c44 Bug: 601198, 762564 Change-Id: I7c229f686ea5f6672b2d4d972a010f2bfbccd40c Reviewed-on: https://chromium-review.googlesource.com/912988 Reviewed-by: Adenilson Cavalcanti <cavalcantii@chromium.org> Reviewed-by: Leon Scroggins <scroggo@chromium.org> Commit-Queue: Noel Gordon <noel@chromium.org> Cr-Commit-Position: refs/heads/master@{#536249} [modify] https://crrev.com/0342258175d72113ae72c0ce56d79a3784ad2dd0/third_party/WebKit/Source/platform/BUILD.gn
,
Feb 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9f16a173c9f23cce777a78db5a278d21bf26258b commit 9f16a173c9f23cce777a78db5a278d21bf26258b Author: Noel Gordon <noel@chromium.org> Date: Tue Feb 13 21:03:24 2018 [image_decode_bench] Introduce ImageMeta data Define and use an ImageMeta structure: to hold meta data about images we decode and, in particular, the image decode time. Use that time to accumlate and report the image decode bench result. Note: the decoder creation time cost is now excluded from the result. Also add a DecodeFailure helper: to simplify decode failure handling, and throw more code away. Errors in the bench are fatal so the helper reports a failure reason and exits. Bug: 601198 Change-Id: Iee7e578d91e705eb3289aac45a2e0b0d75807257 Reviewed-on: https://chromium-review.googlesource.com/914215 Commit-Queue: Noel Gordon <noel@chromium.org> Reviewed-by: Leon Scroggins <scroggo@chromium.org> Cr-Commit-Position: refs/heads/master@{#536470} [modify] https://crrev.com/9f16a173c9f23cce777a78db5a278d21bf26258b/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp
,
Mar 7 2018
From OP > "Whenever possible, avoid creating new benchmarks"), but I think it's reasonable to at least keep it from bit-rotting so developers can use it locally. Yes. > Noel and I have used it a lot, and others have expressed interest in a benchmark that just times image decodes." In the recently zlib work, bench marking with the new zlib_bench [1] added in issue 798943 has been very good way for us to vet optimizations locally, and also for external contributors to do the same. [1] https://cs.chromium.org/search/?q=zlib_bench.cc&sq=package:chromium&type=cs It is a rate-based bench mark tool, with statistics over kRuns. As more developers use the bench tools, we wanna be sure they are measuring correctly (low variance results, aka low jitter) [2]. I re-wrote image_decode_bench during this zlib work to capture variance also and I now propose that we move toward that type of bench marker, and remove the old code [iterations] [packeSize] [2] I have examples of where this was not the case: patches have been allowed to travel to stable channel with regressions in PNG.
,
Mar 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/355e21674ff6172240118a37d7419fe44c0ca7de commit 355e21674ff6172240118a37d7419fe44c0ca7de Author: Noel Gordon <noel@chromium.org> Date: Wed Mar 07 22:36:15 2018 [image_decode_bench] Remove [packetSize] option Patch to prepare to for the removal of [iterations] option, so we can replace it with a rate-based bench marker tool similar to zlib_bench. First step: remove the [packetSize] option. Bug: 601198 Change-Id: If5fabb8597209dac88b134a71d648a950910c86d Reviewed-on: https://chromium-review.googlesource.com/952603 Reviewed-by: Leon Scroggins <scroggo@chromium.org> Commit-Queue: Noel Gordon <noel@chromium.org> Cr-Commit-Position: refs/heads/master@{#541616} [modify] https://crrev.com/355e21674ff6172240118a37d7419fe44c0ca7de/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp
,
Mar 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b8e15ca8e63e539beeeb1d67c14b5d6de590c1d6 commit b8e15ca8e63e539beeeb1d67c14b5d6de590c1d6 Author: Noel Gordon <noel@chromium.org> Date: Fri Mar 09 14:34:38 2018 [image_decode_bench] Remove use of SharedBuffer::Data() SharedBuffer::Data() is deprecated, and was being used to consolidate the buffer into one contiguous piece of memory. This is now redundant since the ShardBuffer is created in ReadFile() by adopting the Vector of image data (which is a contiguous piece of memory under the hood). Remove the SharedBuffer::Data() call, move the related error handling for ReadFile() back into that routine. Bug: 601198 Change-Id: Ic0eab97ead78a967180d1ee6bdcb708e3cc3a4e5 Reviewed-on: https://chromium-review.googlesource.com/956644 Reviewed-by: Leon Scroggins <scroggo@chromium.org> Commit-Queue: Noel Gordon <noel@chromium.org> Cr-Commit-Position: refs/heads/master@{#542102} [modify] https://crrev.com/b8e15ca8e63e539beeeb1d67c14b5d6de590c1d6/third_party/WebKit/Source/platform/testing/ImageDecodeBench.cpp |
|||
►
Sign in to add a comment |
|||
Comment 1 by noel@chromium.org
, Apr 9 2016These seems like good first steps to begin with - get the benchmark building again - support it with gn (I had to use gyp to get it to build) You have a change to get it building with gyp again?