New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 601830 link

Starred by 0 users

Issue metadata

Status: Duplicate
Merged: issue 601799
Owner:
Last visit > 30 days ago
Closed: Apr 2016
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

3%-12.4% regression in blink_perf.canvas at 385474:385520

Project Member Reported by rsch...@chromium.org, Apr 8 2016

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=601830

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgmLW6swoM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg6O_JrQkM


Bot(s) for this bug's original alert(s):

android-nexus5
android-nexus6
One alert maybe recovered, one not. Submitting another bisect.
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Apr 12 2016

Mergedinto: 601799
Status: Duplicate (was: Assigned)

===== 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