New issue
Advanced search Search tips

Issue 904846 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Nov 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Regression



Sign in to add a comment

43ms startup performance regression in gl::Context::finish()

Project Member Reported by wittman@chromium.org, Nov 13

Issue description

Data from the UMA Sampling Profiler shows that the canary population experienced an average regression of 43ms within gl::Context::finish() in gpu::GpuInit::InitializeAndStartSandbox(), in

72.0.3606.0 canary
Windows 64-bit
GPU process main thread

The regression was detected between versions 72.0.3605.0 and 72.0.3606.0.

The following CL is suspected to have caused the regression because it touched functions that regressed or related code:

revision: 905ee08219faed8ea50e194be0d76ad69faf1026
summary: Vulkan: Fix cleanup race condition on Context destroy
author: syoussefi@chromium.org

Please refer to the detailed execution profile difference for gl::Context::finish() to understand what caused the regression: https://uma.googleplex.com/p/chrome/callstacks?sid=8e50269643081ebe3d03d9850f21b523


I'm not sure if this is WAI; assigning for investigation.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 14

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

commit fca8fd6201901e372ef544a847284b0ea02cd1b1
Author: Shahbaz Youssefi <syoussefi@chromium.org>
Date: Wed Nov 14 17:42:09 2018

Vulkan: Refix cleanup race condition on Context destroy

This partially reverts commit 905ee08219faed8ea50e194be0d76ad69faf1026
due to regression caused on startup time.

Instead of calling finish before destroying the context, the objects in
the Vulkan backend are `release()`ed instead of `destroy()`ed, so they
will be kept alive for the duration of current work.

Bug:  904846 
Change-Id: Ia774470666c4c0d4c1ddc348f685d621243de204
Reviewed-on: https://chromium-review.googlesource.com/c/1333969
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Reviewed-by: Geoff Lang <geofflang@chromium.org>
Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>

[modify] https://crrev.com/fca8fd6201901e372ef544a847284b0ea02cd1b1/src/libANGLE/renderer/d3d/d3d11/Renderer11.cpp
[modify] https://crrev.com/fca8fd6201901e372ef544a847284b0ea02cd1b1/src/libANGLE/renderer/d3d/d3d9/Renderer9.cpp
[modify] https://crrev.com/fca8fd6201901e372ef544a847284b0ea02cd1b1/src/libANGLE/Context.cpp
[modify] https://crrev.com/fca8fd6201901e372ef544a847284b0ea02cd1b1/src/libANGLE/renderer/vulkan/VertexArrayVk.cpp
[modify] https://crrev.com/fca8fd6201901e372ef544a847284b0ea02cd1b1/src/libANGLE/renderer/vulkan/vk_helpers.cpp
[modify] https://crrev.com/fca8fd6201901e372ef544a847284b0ea02cd1b1/src/libANGLE/renderer/vulkan/FramebufferVk.cpp
[modify] https://crrev.com/fca8fd6201901e372ef544a847284b0ea02cd1b1/src/libANGLE/renderer/vulkan/vk_helpers.h

Cc: wittman@chromium.org
@wittman, I landed a CL that removes the `finish()` from `Context::destroy()` which should undo this regression. Please keep an eye and let me know once the fix is verified.

FYI, the inefficiency comes from the test for EGL_KHR_surfaceless_context for which we create a context to see if the companion GL_OES_surfaceless_context also exists. A future optimization there could be to defer evaluation the corresponding flag to context creation time, if possible.
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 14

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

commit d483396d85ca738c0aef8d81dbafcd06ee2d9588
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Wed Nov 14 19:45:32 2018

Roll src/third_party/angle d1a55e393e65..fca8fd620190 (2 commits)

https://chromium.googlesource.com/angle/angle.git/+log/d1a55e393e65..fca8fd620190


git log d1a55e393e65..fca8fd620190 --date=short --no-merges --format='%ad %ae %s'
2018-11-14 syoussefi@chromium.org Vulkan: Refix cleanup race condition on Context destroy
2018-11-14 syoussefi@chromium.org Vulkan: prepare for ES3


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

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:904846 
TBR=cwallez@chromium.org

Change-Id: I53bef44de36347ad8e04b502c461208a7c35e581
Reviewed-on: https://chromium-review.googlesource.com/c/1335777
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@{#608091}
[modify] https://crrev.com/d483396d85ca738c0aef8d81dbafcd06ee2d9588/DEPS

Status: Verified (was: Assigned)
This looks to be fixed in 72.0.3611.0, with a reduction of 36ms of execution time: https://uma.googleplex.com/p/chrome/callstacks?sid=0a6a1887dbb9c01cd9fd279ca5602d1c

(The 7ms difference between the regression and the fix is likely due to normal performance variation over clients on the canary channel.)
Thanks for verifying.
Make that https://uma.googleplex.com/p/chrome/callstacks?sid=53a36cd41fcb90401848c1089b4e2e76 which filters to just the gl::Context::finish stack samples.

Sign in to add a comment