Issue metadata
Sign in to add a comment
|
3%-12.4% regression in blink_perf.canvas at 385474:385520 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Apr 11 2016
One alert maybe recovered, one not. Submitting another bisect.
,
Apr 12 2016
===== 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@385440 41710.6 167.36278 5 good chromium@385457 41773.8 140.189871 5 good chromium@385465 41790.6 114.964342 5 good chromium@385469 41750.4 64.11942 5 good chromium@385470 42477.8 84.200356 5 bad <- chromium@385471 42591.8 59.830594 5 bad chromium@385473 42675.8 86.456347 5 bad Bisect job ran on: android_nexus6_perf_bisect Bug ID: 601830 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --also-run-disabled-tests page_cycler.intl_ko_th_vi Test Metric: vm_private_dirty_final_renderer/vm_private_dirty_final_renderer Relative Change: 2.31% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2123 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9015659857487814208 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=601830 | 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! |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by rsch...@chromium.org
, Apr 8 2016