18.9%-168.9% regression in image_decoding.image_decoding_measurement at 385441:385532 |
|||
Issue descriptionSee the link to graphs below.
,
Apr 8 2016
This is almost definitely 33cfb9bd4aec6d9ae36231057436c78b47de8585, confirmed by the fact that it was reverted and it's already fixed on some of the graphs. scroggo, no immediate action for you, but if you plan to reland the change please address these regressions. Thanks!
,
Apr 8 2016
=== Auto-CCing suspected CL author scroggo@chromium.org === Hi scroggo@chromium.org, the bisect results pointed to your CL below as possibly causing a regression. Please have a look at this info and see whether your CL be related. ===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Eliminate copies of encoded image data Author : scroggo Commit description: Remove the ThreadSafeDataTransport, which required making two copies of the encoded data. Instead, copy the data (once) into an SkRWBuffer, which allows creating read only snapshots that can be safely read in another thread. Design doc can be found at https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhSUw/edit#heading=h.hvbfr8rfyj9k Benchmark results can be found at https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7OQOqEo/ DeferredImageDecoder: Copy into an SkRWBuffer, and create snapshots to pass to each DecodingImageGenerator. DecodingImageGenerator: Store a snapshot of the data, along with whether the snapshot contains all of the data. Pass this to the ImageFrameGenerator when it is time to decode. Since it already knows whether it has received all the data, never attempt to YUV decode if not. When created from an SkData, skip the copy into a SharedBuffer. Instead, wrap in a SegmentReader. ImageFrameGenerator: Its decoding APIs now take the encoded data and whether it's complete as parameters. Does not own the encoded data, and no longer has the implementation of onRefEncodedData. Make the object thread-safe where possible, so we can only lock the mutex during operations that truly need it. SegmentReader: Add a new interface that looks like a read only SharedBuffer. Add three implementations: - SharedBufferSegmentReader: Used when we already have a SharedBuffer (e.g. in the main thread). - ROBufferSegmentReader: Facilitates decoding in a worker thread without requiring mutex locks/ an extra copy of the data. - DataSegmentReader: Allows using an SkData directly without copying. ImageDecoders/FastSharedBufferReader: Read data from a SegmentReader instead of directly from a SharedBuffer. (Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing the need to change code that already has a SharedBuffer.) Since the API is the same, the changes here are small, except for WEBPImageDecoder, which no longer calls SharedBuffer::data(), and instead calls SegmentReader::refAsSkData(). Use PassRefPtr instead of raw pointers for parameter passing in setData. Tests: Adapt where a SharedBuffer is no longer used. PictureSnapshot.cpp Wrap SkData in SegmentReader. BUG= 467772 BUG= 483369 BUG= 335694 This may also help with crrbug.com/335694. The DecodingImageGenerator will no longer attempt to read beyond the data that had been received when it was created. However, it theoretically could use a decoder that had previously been used to decode more data than it was created with. e.g. DeferredImageDecoder did = ... did.setData(...); SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator did.setData(...); // More data SkImage imageB = did.createFrameAtIndex(0); // draw imageB - this decodes to the end of data provided in the second // call to setData // draw imageA - this can use the same ImageDecoder retrieved from the // ImageDecodingStore, meaning it will have already // decoded beyond the data supplied to imageA I do not know whether this can happen in practice. Review URL: https://codereview.chromium.org/1812273003 Cr-Commit-Position: refs/heads/master@{#385470} Commit : 33cfb9bd4aec6d9ae36231057436c78b47de8585 Date : Wed Apr 06 15:55:09 2016 ===== TESTED REVISIONS ===== Revision Mean Value Std. Dev. Num Values Good? chromium@385468 152.29246 2.06323 5 good chromium@385469 150.80734 1.894209 5 good chromium@385470 381.16021 4.204548 5 bad <- chromium@385471 383.46473 3.495017 5 bad Bisect job ran on: android_nexus6_perf_bisect Bug ID: 601799 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --also-run-disabled-tests image_decoding.image_decoding_measurement Test Metric: ImageDecoding_avg/ImageDecoding_avg Relative Change: 151.79% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2114 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9015936565168436944 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=601799 | O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq | X | for more information addressing perf regression bugs. For feedback, | / \ | file a bug with component Tests>AutoBisect. Thank you!
,
Apr 14 2016
FYI: I believe I have fixed the performance issue, and I have resubmitted the above CL (https://chromium.googlesource.com/chromium/src/+/d2234904faee943bd987bd38d620096db808efca). I'll watch the dashboard for changes. |
|||
►
Sign in to add a comment |
|||
Comment 1 by rsch...@chromium.org
, Apr 8 2016