New issue
Advanced search Search tips

Issue 651101 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

6.1% regression in angle_perftests/DrawCallPerf_d3d11_null/score on chromium-rel-win7-gpu-nvidia at 420670:420712

Project Member Reported by jmad...@chromium.org, Sep 28 2016

Issue description

Performance dashboard identified a 6.1% regression in angle_perftests/DrawCallPerf_d3d11_null/score on chromium-rel-win7-gpu-nvidia at revision range 420670:420712. Graph: https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=chromium-rel-win7-gpu-nvidia&tests=angle_perftests%2FDrawCallPerf_d3d11_null%2Fscore&checked=score%2Cscore_ref%2Cref&rev=420712

 
Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Sep 29 2016

Cc: tedc...@chromium.org
Owner: tedc...@chromium.org

=== Auto-CCing suspected CL author tedchoc@chromium.org ===

Hi tedchoc@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 : Ensure CCTs have the proper context menu on tab restore.
Author  : tedchoc
Commit description:
  
Custom tab context menus worked previously because CustomTabActivity
was in charge of creating the initial tab, and all subsequent tabs
reuse the delegate of the parent tab.

When restoring, the TabModelSelectorImpl via TabPersistentStore
was creating the tabs and using the default logic in ChromeTabCreator.

This change moves ChromeTabCreator creation to the activities
themselves, which allows CCTs to overwrite the default tab delegate
construction.  This also moves the svelte tab saving logic to the
selector, which is consistent with all other tab saving behavior (and
allows the activity to be unaware of that dependency and enables
creation w/ fewer params).

BUG=649139

Review-Url: https://codereview.chromium.org/2359923005
Cr-Commit-Position: refs/heads/master@{#420702}
Commit  : aaaf17110586e60d241acf02148f9be2dc683015
Date    : Fri Sep 23 19:49:48 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N   Good?
chromium@420669  30097.4  147.559  27  good
chromium@420691  30082.1  148.309  27  good
chromium@420697  30033.8  204.475  18  good
chromium@420700  30096.8  191.535  12  good
chromium@420701  30095.5  99.526   8   good
chromium@420702  29587.3  552.0    12  bad    <--
chromium@420712  29896.4  271.212  18  bad

Bisect job ran on: winx64nvidia_perf_bisect
Bug ID: 651101

Test Command: .\src\out\Release_x64\angle_perftests.exe --test-launcher-print-test-stdio=always --test-launcher-jobs=1
Test Metric: DrawCallPerf_d3d11_null/score
Relative Change: 1.03%
Score: 99.0

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64nvidia_perf_bisect/builds/1883
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9000262864991858800


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

| 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!
Owner: ----
Java only change, so can't impact win.  One of the rare times, that I can say with 100% confidence that this was not me.
Perf bisect, come on buddy. :( I'll try it again.
I nudged it forward into a range with two angle rolls.
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Sep 30 2016

Cc: cwallez@chromium.org
Owner: cwallez@chromium.org

=== Auto-CCing suspected CL author cwallez@chromium.org ===

Hi cwallez@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 : Implement a separate last row texture unpack buffer upload workaround
Author  : Corentin Wallez
Commit description:
  
When uploading textures from an unpack buffer, some drivers expect an
extra row paading, causing them to think the pixel buffer is not big enough.
We work around this by uploading the last row separately.

BUG= angleproject:1512 

Change-Id: I52fb8b35dc450b957f1fafb0b405c81bf0504157
Reviewed-on: https://chromium-review.googlesource.com/385193
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit  : c5cacd6035ecd0882c2bb483c4ca50380b835c4c
Date    : Thu Sep 22 14:39:43 2016


===== TESTED REVISIONS =====
Revision                          Mean     Std Dev  N  Good?
chromium@420712                   30054.6  100.912  5  good
chromium@420728                   30072.8  56.6586  5  good
chromium@420730                   30117.0  57.2844  5  good
chromium@420731                   30128.0  40.4166  5  good
chromium@420731,angle@44d0a73654  29777.0  203.392  5  good
chromium@420731,angle@c5cacd6035  28208.6  189.446  5  bad    <--
chromium@420731,angle@f979524a0e  29203.6  59.6766  5  bad
chromium@420731,angle@596018ce7b  28765.6  164.19   5  bad
chromium@420732                   28287.2  124.586  5  bad
chromium@420736                   28297.2  113.676  5  bad
chromium@420744                   28240.2  51.9394  5  bad
chromium@420776                   28258.4  154.204  5  bad

Bisect job ran on: winx64nvidia_perf_bisect
Bug ID: 651101

Test Command: .\src\out\Release_x64\angle_perftests.exe --test-launcher-print-test-stdio=always --test-launcher-jobs=1
Test Metric: DrawCallPerf_d3d11_null/score
Relative Change: 5.98%
Score: 99.5

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64nvidia_perf_bisect/builds/1885
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9000166051286035136


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

| 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!
Corentin, maybe we can fix this using the caching we discussed in person?
I don't see how the CL could be related to this regression because:
 - Most of the changes are in the GL backend, the regression is for D3D11
 - The change to validationES3 is for texImage2D, under pixelUnpackBuffer != nullptr and the test only uses texImage2D once, without pixel unpack buffers.
 - The only thing left is the change to computeUnpackSize, which is a bugfix (and I don't see how it would get called from DrawCallPerf).

On the other hand, the parent commit, 44d0a73654 adds conditions and string compares to DrawCallPerf.
Nevermind that last paragraph, it was in DrawBuffersTest instead.
It might be the small change to computeRowPitch messed up the optimization somehow. I guess close as WontFix if you can't reproduce this.
Status: Started (was: Untriaged)
I was able to reproduce the regression and will make a fix by not computing rowPitch twice when computeUnpackSize is called.
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2ae51c4ea3e7b08e143df3a97017519bd3e2ca11

commit 2ae51c4ea3e7b08e143df3a97017519bd3e2ca11
Author: jmadill <jmadill@chromium.org>
Date: Tue Oct 04 17:15:21 2016

Roll ANGLE 886de36..4c65524

https://chromium.googlesource.com/angle/angle.git/+log/886de36..4c65524

BUG= chromium:651493 , 651101 , chromium:618464 

TBR=geofflang@chromium.org

TEST=bots

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2393613002
Cr-Commit-Position: refs/heads/master@{#422837}

[modify] https://crrev.com/2ae51c4ea3e7b08e143df3a97017519bd3e2ca11/DEPS

Status: Fixed (was: Started)
Looking at the graph, it seems this fixed half of the regression, but I don't think we'll be able to get back the other half, because computeUnpackSize used to be too simple, and wrong.
Great, there's some variance to the tests as well. Thanks for addressing this.

Sign in to add a comment