New issue
Advanced search Search tips

Issue 674143 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

27.3% regression in angle_perftests at 438149:438200

Project Member Reported by jmad...@chromium.org, Dec 14 2016

Issue description

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

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


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

chromium-rel-win7-gpu-nvidia
Components: Internals>GPU>ANGLE
Owner: cwallez@chromium.org
Corentin, I narrowed this to:

Tighten the validation for staying in array buffer bounds

BUG=angleproject:1523

Change-Id: I685a9b884e5de73b94900c5b6016ce914b9bfe7f
Reviewed-on: https://chromium-review.googlesource.com/418458
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Geoff Lang <geofflang@chromium.org>

Do you think there's some optimization we can do here? 27% is a fairly substantial chunk. Can we do some caching somewhere?
Ew.

There's some gains that could be had by not using CheckedNumeric and do things manually instead. From a profiler run on Cesium I found that a third of the time in ValidateDrawAttribs is in CheckedMul.

Other things we could do are:
 - Assess the robustness of the backends and don't do bounds checking if possible.
 - Cache the validation in the VertexArray somehow, the tricky part being that the validation doesn't only depend on the VAO state, but also on the maximum vertex and the number of instances.
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Dec 15 2016

Cc: cwallez@chromium.org

=== PERF REGRESSION ===


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

Hi cwallez@chromium.org, the bisect results pointed to your CL, please take a look at the
results.


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


===== SUSPECTED CL(s) =====
Subject : Tighten the validation for staying in array buffer bounds
Author  : Corentin Wallez
Commit description:
  
BUG=angleproject:1523

Change-Id: I685a9b884e5de73b94900c5b6016ce914b9bfe7f
Reviewed-on: https://chromium-review.googlesource.com/418458
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Geoff Lang <geofflang@chromium.org>
Commit  : 92db69472bad2344d99b93a470053d73eb13d062
Date    : Mon Dec 12 16:54:42 2016


===== TESTED REVISIONS =====
Revision                          Mean    Std Dev  N  Good?
chromium@438148                   190973  4235.32  6  good
chromium@438155                   187821  6789.66  6  good
chromium@438158                   188414  5262.94  6  good
chromium@438160                   189315  5173.27  6  good
chromium@438160,angle@f52fe93dca  191646  1916.72  6  good
chromium@438160,angle@92db69472b  139471  1226.05  6  bad    <--
chromium@438160,angle@a2c7498568  135008  1045.67  6  bad
chromium@438160,angle@36fd100d48  140195  3322.49  6  bad
chromium@438161                   136921  1545.63  6  bad
chromium@438174                   137106  883.272  6  bad
chromium@438200                   136542  1253.42  6  bad

Bisect job ran on: winx64nvidia_perf_bisect
Bug ID: 674143

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

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


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

| 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: jmad...@chromium.org
27% regression is unacceptable. I'm going to try and reduce this.
Owner: cwallez@chromium.org
Corentin said he's working on this.
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/angle/angle/+/71168a07faaadc6896d8a634fd8862d8709a58c9

commit 71168a07faaadc6896d8a634fd8862d8709a58c9
Author: Corentin Wallez <cwallez@chromium.org>
Date: Mon Dec 19 23:11:18 2016

Manually write overflow checks for glDraw*

The usage of checked numerics showed has a big hotspot in ValidateDrawAttribs.
BUG= angleproject:1671 
BUG= chromium:674143 

Change-Id: I96392e099b2257e465fb47e1c96f9aa1f54a89ba
Reviewed-on: https://chromium-review.googlesource.com/422428
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Geoff Lang <geofflang@chromium.org>

[modify] https://crrev.com/71168a07faaadc6896d8a634fd8862d8709a58c9/src/libANGLE/validationES.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 20 2016

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

commit 47cd7960532e2a1bd4e7a987b69c66f632f7f6c9
Author: jmadill <jmadill@chromium.org>
Date: Tue Dec 20 19:12:22 2016

Roll ANGLE f047097..71168a0

https://chromium.googlesource.com/angle/angle.git/+log/f047097..71168a0

BUG= chromium:674143 

TBR=cwallez@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/2597443002
Cr-Commit-Position: refs/heads/master@{#439866}

[modify] https://crrev.com/47cd7960532e2a1bd4e7a987b69c66f632f7f6c9/DEPS

Project Member

Comment 10 by bugdroid1@chromium.org, Dec 20 2016

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

commit 67a71ece49fdbe3ef686ea54ded42f9523f418e5
Author: jam <jam@chromium.org>
Date: Tue Dec 20 20:12:50 2016

Revert of Roll ANGLE f047097..71168a0 (patchset #1 id:1 of https://codereview.chromium.org/2597443002/ )

Reason for revert:
https://build.chromium.org/p/chromium/builders/Win%20x64/builds/6994/steps/compile/logs/stdio

ninja -t msvc -e environment.x64 -- C:\b\c\cipd\goma/gomacc.exe "C:\b\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\VC\bin\amd64/cl.exe" /nologo /showIncludes /FC @obj/third_party/angle/src/tests/angle_perftests/UniformsPerf.obj.rsp /c ../../third_party/angle/src/tests/perf_tests/UniformsPerf.cpp /Foobj/third_party/angle/src/tests/angle_perftests/UniformsPerf.obj /Fd"obj/third_party/angle/src/tests/angle_perftests_cc.pdb"
c:\b\c\b\win_x64_archive\src\third_party\angle\src\tests\perf_tests\uniformsperf.cpp(148): error C2220: warning treated as error - no 'object' file generated
c:\b\c\b\win_x64_archive\src\third_party\angle\src\tests\perf_tests\uniformsperf.cpp(148): warning C4267: 'initializing': conversion from 'size_t' to 'GLint', possible loss of data
c:\b\c\b\win_x64_archive\src\third_party\angle\src\tests\perf_tests\uniformsperf.cpp(149): warning C4267: 'initializing': conversion from 'size_t' to 'GLint', possible loss of data

Original issue's description:
> Roll ANGLE f047097..71168a0
>
> https://chromium.googlesource.com/angle/angle.git/+log/f047097..71168a0
>
> BUG= chromium:674143 
>
> TBR=cwallez@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
>
> Committed: https://crrev.com/47cd7960532e2a1bd4e7a987b69c66f632f7f6c9
> Cr-Commit-Position: refs/heads/master@{#439866}

TBR=cwallez@chromium.org,jmadill@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= chromium:674143 

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

[modify] https://crrev.com/67a71ece49fdbe3ef686ea54ded42f9523f418e5/DEPS

Project Member

Comment 11 by bugdroid1@chromium.org, Dec 20 2016

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

commit b5de482ad9cc882bf3ef9a73f3d75953a3350c44
Author: jmadill <jmadill@chromium.org>
Date: Tue Dec 20 23:15:24 2016

Roll ANGLE f047097..d901983

https://chromium.googlesource.com/angle/angle.git/+log/f047097..d901983

BUG=None,chromium:668223,chromium:674143

TBR=cwallez@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/2589343002
Cr-Commit-Position: refs/heads/master@{#439916}

[modify] https://crrev.com/b5de482ad9cc882bf3ef9a73f3d75953a3350c44/DEPS

Status: Fixed (was: Assigned)
Excellent work Corentin. The test seems to have recovered.
Thanks!

Sign in to add a comment