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

Issue 601820 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

2.3%-8.5% regression in page_cycler.intl_ko_th_vi at 385445:385532

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

Issue description

See the link to graphs below.
 

===== BISECT JOB RESULTS =====
Status: completed


=== Bisection aborted ===
The bisect was aborted because The metric values for the initial "good" and "bad" revisions do not represent a clear regression.
Please contact the the team (see below) if you believe this is in error.

=== Warnings ===
The following warnings were raised by the bisect job:

 * Bisect failed to reproduce the regression with enough confidence.

===== TESTED REVISIONS =====
Revision                Mean Value  Std. Dev.   Num Values  Good?
chromium@385467         38.850495   0.456644    12          good
chromium@385470         38.647233   0.599368    18          bad

Bisect job ran on: win_perf_bisect
Bug ID: 601820

Test Command: .\src\out\Release\performance_browser_tests.exe --test-launcher-print-test-stdio=always --enable-gpu
Test Metric: CastV2Performance_gpu_30fps_slow/time_between_frames
Relative Change: 0.00%
Score: 0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/6453
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9015930763367050416


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=601820

| 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!
submitted some more bisects
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Apr 16 2016


===== BISECT JOB RESULTS =====
Status: completed


=== Bisection aborted ===
The bisect was aborted because The metric values for the initial "good" and "bad" revisions do not represent a clear regression.
Please contact the the team (see below) if you believe this is in error.

=== Warnings ===
The following warnings were raised by the bisect job:

 * Bisect failed to reproduce the regression with enough confidence.

===== TESTED REVISIONS =====
Revision                Mean Value  Std. Dev.   Num Values  Good?
chromium@385453         37.396643   0.442761    12          good
chromium@385500         37.452919   0.4306      18          bad

Bisect job ran on: win_perf_bisect
Bug ID: 601820

Test Command: .\src\out\Release\performance_browser_tests.exe --test-launcher-print-test-stdio=always --enable-gpu
Test Metric: CastV2Performance_gpu_30fps_slow/time_between_captures
Relative Change: 0.11%
Score: 0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/6469
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9015282665255702640


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=601820

| 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!
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Apr 16 2016


===== BISECT JOB RESULTS =====
Status: completed


=== Bisection aborted ===
The bisect was aborted because The metric values for the initial "good" and "bad" revisions do not represent a clear regression.
Please contact the the team (see below) if you believe this is in error.

=== Warnings ===
The following warnings were raised by the bisect job:

 * Bisect failed to reproduce the regression with enough confidence.

===== TESTED REVISIONS =====
Revision                Mean Value  Std. Dev.   Num Values  Good?
chromium@385453         37.457046   0.393938    12          good
chromium@385500         37.322474   0.446537    18          bad

Bisect job ran on: win_perf_bisect
Bug ID: 601820

Test Command: .\src\out\Release\performance_browser_tests.exe --test-launcher-print-test-stdio=always --enable-gpu
Test Metric: CastV2Performance_gpu_30fps_slow/time_between_frames
Relative Change: 1.24%
Score: 0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/6470
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9015282654068205728


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=601820

| 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!
Project Member

Comment 6 by 42576172...@developer.gserviceaccount.com, Apr 16 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@385458         76303.4     65.500382   5           good
chromium@385464         76354.2     509.318859  5           good
chromium@385467         76172.4     264.4793    5           good
chromium@385469         76083.6     136.192144  5           good
chromium@385470         78662.4     870.090972  5           bad         <-

Bisect job ran on: android_nexus9_perf_bisect
Bug ID: 601820

Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --also-run-disabled-tests page_cycler.basic_oopif
Test Metric: vm_private_dirty_final_renderer/vm_private_dirty_final_renderer
Relative Change: 3.09%
Score: 99.5

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus9_perf_bisect/builds/1740
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9015282662664823584


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=601820

| 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!
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Apr 16 2016


===== BISECT JOB RESULTS =====
Status: completed


=== Bisection aborted ===
The bisect was aborted because The metric values for the initial "good" and "bad" revisions do not represent a clear regression.
Please contact the the team (see below) if you believe this is in error.

=== Warnings ===
The following warnings were raised by the bisect job:

 * Bisect failed to reproduce the regression with enough confidence.

===== TESTED REVISIONS =====
Revision                Mean Value  Std. Dev.   Num Values  Good?
chromium@385466         43530.5     187.832638  6           good
chromium@385532         44324.166667119.379088  6           bad

Bisect job ran on: android_s5_perf_bisect
Bug ID: 601820

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: 1.77%
Score: 0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_s5_perf_bisect/builds/605
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9015282659829994000


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=601820

| 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