New issue
Advanced search Search tips

Issue 896406 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Yesterday
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

215ms startup performance regression in gl::ValidateUseProgram()

Project Member Reported by wittman@chromium.org, Oct 17

Issue description

Suspect CL
revision: 44a6fbfd95dd36761ddbf705684683b41cb7c6b1
summary: Optimize resolveLink.
author: jmadill@chromium.org

This CL is suspected of introducing an average regression of 215ms over the canary population during the first 30 seconds of startup in

71.0.3570.0 canary
Windows 64-bit
GPU process main thread

The regression was detected between versions 71.0.3569.0 and 71.0.3570.0.

Execution profile for gl::ValidateUseProgram(): https://uma.googleplex.com/p/chrome/callstacks?sid=eb8e92f6dc0d4940c04c09fb55c750b0

 
Labels: -Type-Bug-Regression Type-Bug
Status: WontFix (was: Assigned)
Thinking about this more. There's no way to avoid resolving the link in UseProgram. I expect this regression is not really a regression but a shifting of time from one function to another. It just happened to trip the automatic alerting. Good to know that works though.
This seems to be partially but not fully explained by shifting execution -- in the full first 30 seconds of startup there's a residual 97ms increase in execution time. See the wWinMain delta here: https://uma.googleplex.com/p/chrome/callstacks?sid=9d5b72fd2a53deb92aa91ba00532f6fd.

As a side note, the GPU profiling data can be noisy but the difference appears to remain when comparing earlier and later releases so I suspect it's legitimate.
The graph seems a bit fishy to me.

Inspecting the CL in question:

https://chromium-review.googlesource.com/c/angle/angle/+/1255509/7/src/libANGLE/validationES2.cpp#6340

Here we see ValidateUseProgram calls isLinked as long as the Program is valid.

https://chromium-review.googlesource.com/c/angle/angle/+/1255509/7/src/libANGLE/Program.cpp#b1280

Here we see the "old" isLinked always calls resolveLink.

In the graph I don't see where the "old" resolveLink happens. But the Program's link should be resolved by UseProgram in both cases. Is it possible the resolveLink is being called in the "old" trace before we even get to this flame graph? Or is there some systematic problem with comparing these different stack traces?
This is due to a limitation of the flame graph difference display: differences where stacks have completely disappeared between the versions are not visible because they are superimposed "new" version profile which doesn't have those stacks. They're visible in the tree view however: https://uma.googleplex.com/p/chrome/callstacks?sid=78abc37d4d61eeacb156bc4cd78b6cf5

The other issue is that resolveLink() has been inlined, so doesn't appear directly; we only see resolveLinkImpl().
Cc: syoussefi@chromium.org
Cc: jmad...@chromium.org
Owner: syoussefi@chromium.org
Status: Assigned (was: WontFix)
Looks like this might in fact be related to a change Shabi made:

https://chromium-review.googlesource.com/c/1251325

Shabi could you investigate?
Project Member

Comment 7 by bugdroid1@chromium.org, Yesterday (38 hours ago)

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

commit 37d2e606c6000985f886a8575526cc3a8cd930b6
Author: Shahbaz Youssefi <syoussefi@chromium.org>
Date: Mon Jan 21 18:28:56 2019

Make BlobCache use independent from ANGLE_program_cache_control

The decision to enable caching or not was based on the size of the cache
as directed by eglProgramCacheResizeANGLE, which is incorrect for
applications that only use the ANDROID_blob_cache extension.

The new behavior is as follows:

- If EGL_CONTEXT_PROGRAM_BINARY_CACHE_ENABLED_ANGLE is specifically
  requested and set to true, but the size of the cache is 0, caching is
  disabled for the context.
- If EGL_CONTEXT_PROGRAM_BINARY_CACHE_ENABLED_ANGLE is specifically
  requested and set to false, caching is disabled for the context.
- Otherwise, caching is left enabled for the context, although the cache
  possibly has a size of zero, which effectively disables it.  During
  application execution, if the blob cache callbacks are set, the
  application cache will be used.

Bug:  896406 ,  angleproject:2516 
Change-Id: Ic0cabda03fb6bf53994e86e3ede30afc8021d67e
Reviewed-on: https://chromium-review.googlesource.com/c/1405708
Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
Reviewed-by: Geoff Lang <geofflang@chromium.org>

[modify] https://crrev.com/37d2e606c6000985f886a8575526cc3a8cd930b6/src/libANGLE/Display.cpp
[modify] https://crrev.com/37d2e606c6000985f886a8575526cc3a8cd930b6/src/libANGLE/MemoryProgramCache.cpp
[modify] https://crrev.com/37d2e606c6000985f886a8575526cc3a8cd930b6/src/libANGLE/BlobCache.cpp
[modify] https://crrev.com/37d2e606c6000985f886a8575526cc3a8cd930b6/src/libANGLE/BlobCache.h
[modify] https://crrev.com/37d2e606c6000985f886a8575526cc3a8cd930b6/src/tests/egl_tests/EGLBlobCacheTest.cpp

Comment 8 by syoussefi@google.com, Yesterday (36 hours ago)

Status: Fixed (was: Assigned)

Comment 9 by jmadill@google.com, Yesterday (36 hours ago)

Cc: wittman@chromium.org
Thanks Shabi for the quick fix. This should roll into Chrome here:

https://chromium-review.googlesource.com/c/chromium/src/+/1425959

We should be able to see when this rolls into Canary. Mike would it be possible to watch the sampling profiler for an improvement in process startup of about 200ms? It seems you were right and the regression was legitimate.
Project Member

Comment 10 by bugdroid1@chromium.org, Yesterday (35 hours ago)

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

commit 47cfc8ebe2eb4e984392345d23a82dac81e81452
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Mon Jan 21 21:07:57 2019

Roll src/third_party/angle bf433727853d..c81e7bfeb558 (2 commits)

https://chromium.googlesource.com/angle/angle.git/+log/bf433727853d..c81e7bfeb558


git log bf433727853d..c81e7bfeb558 --date=short --no-merges --format='%ad %ae %s'
2019-01-21 syoussefi@chromium.org Vulkan: refactor CommandGraphResource
2019-01-21 syoussefi@chromium.org Make BlobCache use independent from ANGLE_program_cache_control


Created with:
  gclient setdep -r src/third_party/angle@c81e7bfeb558

The AutoRoll server is located here: https://autoroll.skia.org/r/angle-chromium-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG= chromium:896406 
TBR=syoussefi@chromium.org

Change-Id: Ic871aa4ad2f00e369b88cbbda918e48b69ca19aa
Reviewed-on: https://chromium-review.googlesource.com/c/1425959
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#624673}
[modify] https://crrev.com/47cfc8ebe2eb4e984392345d23a82dac81e81452/DEPS

Sign in to add a comment