Reduce cost of shader compilation in WebRender startup

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P1
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: kamidphish, Assigned: jgilbert)

Tracking

(Blocks 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(7 attachments)

No description provided.
MozReview-Commit-ID: 4RHhlk53R5f
MozReview-Commit-ID: LIH83gKxaw2
Was seeing the creation of debug shaders on creation of a new window with Ctrl+N in gecko.

MozReview-Commit-ID: 9p3D2hLSL8r
MozReview-Commit-ID: 3EHw9hdfKwe
MozReview-Commit-ID: GEfwKbLFetq
(In reply to Dan Glastonbury (:kamidphish) | needinfo me from comment #1)
> Created attachment 9010533 [details] [diff] [review]
> Hack ANGLE to add EGL_MOZ_create_context_provoking_vertex_dont_care
> extension.
> 
> MozReview-Commit-ID: ExeqEnRtmpB

This patch adds the ability to disable ANGLE's generation of geometry shaders to match the OpenGL specified behavior for "flat" attributes between VS-FS. WebRender's shaders don't depend on the provoking vertex order.

This is reported to ANGLE in issue 2829 (https://bugs.chromium.org/p/angleproject/issues/detail?id=2829)
(In reply to Dan Glastonbury (:kamidphish) | needinfo me from comment #2)
> Created attachment 9010534 [details] [diff] [review]
> Use EGL_MOZ_context_create_provoking_vertex_dont_care
> 
> MozReview-Commit-ID: It8fOzqiFIe

Use EGL_MOZ_create_context_provoking_vertex_dont_care extension when creating context for WebRender.
(In reply to Dan Glastonbury (:kamidphish) | needinfo me from comment #3)
> Created attachment 9010535 [details] [diff] [review]
> One, shared GLContext in RenderThread
> 
> MozReview-Commit-ID: 4RHhlk53R5f

Create one GL context for all WebRender to use, owned by the RenderThread.  This is to facilitate the sharing of shaders across all WebRender windows.
(In reply to Dan Glastonbury (:kamidphish) | needinfo me from comment #10)
> Create one GL context for all WebRender to use, owned by the RenderThread. 
> This is to facilitate the sharing of shaders across all WebRender windows.

This change only creates an ANGLE context.  There is more work required to implement CreateGLContextOGL() for non-ANGLE contexts.
(In reply to Dan Glastonbury (:kamidphish) | needinfo me from comment #4)
> Created attachment 9010536 [details] [diff] [review]
> Remove unused _file_changed_handler
> 
> MozReview-Commit-ID: LIH83gKxaw2

_file_changed_handler isn't used. Removing it caused an avalanche of dead code.
(In reply to Dan Glastonbury (:kamidphish) | needinfo me from comment #5)
> Created attachment 9010537 [details] [diff] [review]
> Don't create a debug_renderer if there is nothing to render.
> 
> Was seeing the creation of debug shaders on creation of a new window with
> Ctrl+N in gecko.
> 
> MozReview-Commit-ID: 9p3D2hLSL8r

Debug shaders are created even when there is no debug rendering.

https://github.com/servo/webrender/issues/3089
(In reply to Dan Glastonbury (:kamidphish) | needinfo me from comment #6)
> Created attachment 9010538 [details] [diff] [review]
> Shader pre-caching and sharing amongst windows.
> 
> MozReview-Commit-ID: 3EHw9hdfKwe

Create a Shaders struct early in RenderThread startup and hand it to each created WebRender Renderer created in wr_window_new.
(In reply to Dan Glastonbury (:kamidphish) | needinfo me from comment #7)
> Created attachment 9010539 [details] [diff] [review]
> Add stratified shader precaching
> 
> MozReview-Commit-ID: GEfwKbLFetq

Split the pre-caching concept into two tiers. The "essential" shaders required to get to a basic page like about:newtab and "all" shaders.
https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FeX26uatBTkuAnbX6Ft7_0Q%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_sessionrestore.zip/calltree/?file=profile_sessionrestore%2Fstartup%2Fcycle_3.profile&globalTrackOrder=1-0-2-3-4-5&hiddenGlobalTracks=1&localTrackOrderByPid=6528-1-0~2660-0-1-2-3-4-5-6-7-8-9-10-11-12~&range=0.0000_2.2274~0.0000_1.0608~0.0000_0.4305&thread=7&v=3

Outstanding issue is that shader compilation takes 190ms on RenderThread.

A number of ideas have been canvased to improve the situation, mostly focusing on ideas to parallelize the compilation:

a) Use GL_KHR_parallel_shader_compile extension
b) Create a thread inside wr::Renderer::new() to dispatch shader compilation to
c) Create a thread inside RenderThread to dispatch shader compilation to

Each one has issues.

a) This feature is implemented in ANGLE and gives ANGLE a thread pool to compile shaders upon.  The API requires refactoring the way shaders are loaded to not cause a stall because as soon as an OpenGL function is issues against the program. The only non-blocking call is glGetProgramiv(GL_COMPLETION_STATUS_KHR) which will inform the called if program linking has completed.  To use this in WebRender, Device::create_program will need to be refactored to check the link status on render to delay blocking on waiting for the link to complete.

b) :gw suggested the following:

> ===== current =====
> 
> renderer::new()
> 	let device = ...
> 	create some GL resources with device
> 	let shaders = Shaders::new(&device)
> 	create some more GL resources
> 
> 
> ===== proposed =====
> 
> renderer::new()
> 	let device = ...
> 	create (cheap) non-shader GL resources
> 	let (tx, rx) = channel();
> 	// spawn thread which takes ownership of device
> 	spawn(|| {
> 		let shaders = Shaders::new(&device);
> 		tx.send(shaders, device);
> 	})
> 
> renderer::draw()
> 	// if draw is invoked for the first time without shaders, block until thread returns them
> 	if self.shaders.is_none() {
> 		(self.shaders, self.device) = self.rx.recv().unwrap()
> 	}

The idea was to move Device and Shaders to another thread so that they couldn't be accidentally used.  Unfortunately Device can't be moved because it holds Rc<>.  :gw suggests removing the need to pass the Device to another thread but that requires calling directly to the GL context to do the creation.

c) We are blocking because wr_window_new requires the Device and Shaders at be passed into the creation of wr::Renderer. We could spin up a thread pool and create a GL context per thread that shares the main context and just causes the program to be linked.
Priority: P3 → P2
I'll at least get the ANGLE requirements satisfied:
https://bugs.chromium.org/p/angleproject/issues/detail?id=2829
Assignee: nobody → jgilbert
Priority: P2 → P1
(Tentatively marking this as P1 because it's pretty important, but we could have a conversation about riding to beta without it).
Depends on: 1494474
Depends on: 1494477
So Jamie Madill (Google) has this recommendation, in context of concurrent glProgramBinary:
> JG: What are our other startup options?
> JM: hey, depends on what you're doing, but you can use the ANGLE internal program cache control extension or the blob cache extension
>     i think we're transitioning to the blob cache extension
>     is this binary from an internal cache in webrender?
> JG: Yeah, we load it out of a disk cache
> JM: cool, that's precisely what the blob cache extension can do
>     and the cache control extension too if that one doesn't meet your needs
>     https://www.khronos.org/registry/EGL/extensions/ANDROID/EGL_ANDROID_blob_cache.txt
> JG: We'll take a look!
> JM: https://chromium.googlesource.com/angle/angle/+/master/extensions/ANGLE_program_cache_control.txt
> JG: cool
> JM: oh and this https://chromium.googlesource.com/angle/angle/+/master/extensions/EGL_ANGLE_program_cache_control.txt
> JG: Oh weird, I get it. This covers shader recompilation?
> JM: yeah i think they both get triggers when there is a dynamic shader recompile
>     during a draw call
I took this patch queue, added a prototype for bug 1494474 (async glProgramBinary) plus some WebRender changes to avoid accidentally flushing too early, and it looks like that's going to be sufficient to fix the startup regressions:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0333b04b623a0b6f2835c67d7c9aabed79b17f7

I'll start working on cleaning the patches up so that we can land them asap.
Depends on: 1494763
Depends on: 1495902
Depends on: 1497677
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb58c73f2d5f
Use EGL_MOZ_context_create_provoking_vertex_dont_care. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/cb58c73f2d5f
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.