Introduce uniform bindings #6018

lbud · 2018-01-18T22:42:25Z

This PR

introduces uniform binding classes that manage state for uniform bindings
Introduces render/program/*_program.js files (open to renaming — these may be better for now as *_program_uniforms.js or something) which create uniform bindings for per-layer uniforms (henceforth fixedUniforms) and perform those uniform calculations (moved from draw_* functions)
Refactors Program#draw functions to
- set color, stencil, and depth modes
- set all uniforms (both fixedUniforms and paint property-based uniforms, henceforth binderUniforms; the state bindings for these are initialized upon program construction)
Moves to drawing exclusively with drawElements (no drawArrays) so that all drawing is done through the same Program#draw path
- Introduces SegmentVector#simpleSegment in order to create necessary segments to manage these simple draw calls; we create these in lieu of VAOs in painter setup
Removes almost all of the "first" / "programChanged" logic, hoping that managing uniform state mitigates perf concerns enough (may revisit — see below)

Current benchmark results (updated 3/19): https://bl.ocks.org/anonymous/raw/cc118c1ebafcbb06a7d8fdd8c2b50c8b/

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
post satisfactory benchmark scores (not good enough yet)
manually test the debug page

anandthakker

This is looking great!

The main big-picture question I'm pondering is whether we can/should push the typing further.

I.e.:

For each program type (fill, circle, etc.), declare a base object mapping uniform names to their types.
Parameterize Uniforms, UniformValues, UniformLocations, and Program itself on that base object. (We'd need to figure out how to deal with the dynamically-determined paint property uniforms that come out of ProgramConfiguration#getUniforms().)

anandthakker · 2018-01-24T14:36:07Z

src/render/uniform_binding.js

+export interface UniformInterface<T> {
+    context: Context;
+    set(location: WebGLUniformLocation, value: T): void;
+    _set(location: WebGLUniformLocation, value: T): void;


Is the purpose of _set to allow each concrete uniform subclass to call the correct gl.uniform*() method? If so, rather than having it in this public interface, it can just be declared as a property on the Uniform<T> class _set: (location: WebGLUniformLocation, value: T) => void;

anandthakker · 2018-01-24T14:41:17Z

src/render/uniform_binding.js

+        let diff = false;
+        if (!this.current && v) {
+            diff = true;
+        } else if (Array.isArray(this.current) && Array.isArray(v) && this.current !== v) {


this.current !== v means we're assuming array values are never mutated in place. That's probably a valid assumption, flagging just in case.

🤔 it feels like a valid assumption, is currently true, and would obviously be more performant to proceed as such, although I suppose there's nothing preventing someone from doing this…

Remember how for performance reasons we ended up inlining the this.current tests for Value implementations? Let's do the same for uniforms. In addition to the likely performance advantage, we can reduce the required assumptions by tailoring conditionals to the value type, e.g. for Uniform2f: if (this.current !== v || this.current[0] !== v[0] || this.current[1] !== v[1]) {.

anandthakker · 2018-01-26T14:02:53Z

src/render/draw_fill.js

    }
 }

-function drawFillTiles(painter, sourceCache, layer, coords, drawFn) {
-    if (pattern.isPatternMissing(layer.paint.get('fill-pattern'), painter)) return;
+function drawFillTiles(painter, sourceCache, layer, coords, depthMode, colorMode, fillTiles) {


small clarification: maybe change fillTiles to something like phase: 'fill' | 'outline'?

I'm going to change to isOutline to maintain a simple boolean while being a bit more descriptive.

anandthakker · 2018-01-26T14:37:59Z

src/render/program/background_program.js

+    tile: {tileID: OverscaledTileID, tileSize: number}
+): UniformValues {
+    return util.extend(pattern.prepare(image, painter),
+        pattern.setTile(tile, painter),


Since pattern.prepare() and pattern.setTile() are state-mutating methods, I'm wary of them being called from within backgroundUniformValues(), etc., which seem intended to be "pure" functions that just return computed uniform values.

lbud · 2018-02-28T21:20:18Z

Updated OP with current benchmarks; the Paint benchmark is reliably performing about 2 ms slower than master at the moment :(

jfirebaugh · 2018-02-28T19:25:39Z

src/render/uniform_binding.js

+
+import type Context from '../gl/context';
+
+export interface UniformInterface<T> {


UniformInterface isn't imported anywhere; I think it can be removed (along with the implements below).

jfirebaugh · 2018-02-28T19:40:25Z

src/render/uniform_binding.js

+
+export type UniformValue = number | Array<number> | Float32Array;
+export type UniformValues<Us: Object>
+    = $Exact<$ObjMap<Us, <V>(u: Uniform<V>) => UniformValue>>;


Ideally this would be:

= $Exact<$ObjMap<Us, <V>(u: Uniform<V>) => V>>;

That would permit the type checker to verify that Uniforms<Us>#set is receiving the right types of values in the second parameter.

There are a number of things that need to happen to make this possible. A starting point is changing:

class Uniform2fv extends Uniform<Array<number>> { _set(location: WebGLUniformLocation, v: Array<number>): void { this.context.gl.uniform2fv(location, v); } }

to:

class Uniform2f extends Uniform<[number, number]> { _set(location: WebGLUniformLocation, v: [number, number]): void { this.context.gl.uniform2f(location, v[0], v[1]); } }

(And so on for the other Uniform* classes.)

jfirebaugh · 2018-02-28T19:43:41Z

src/render/uniform_binding.js

+        let diff = false;
+        if (!this.current && v) {
+            diff = true;
+        } else if (Array.isArray(this.current) && Array.isArray(v) && this.current !== v) {


Remember how for performance reasons we ended up inlining the this.current tests for Value implementations? Let's do the same for uniforms. In addition to the likely performance advantage, we can reduce the required assumptions by tailoring conditionals to the value type, e.g. for Uniform2f: if (this.current !== v || this.current[0] !== v[0] || this.current[1] !== v[1]) {.

lbud · 2018-03-07T00:52:13Z

For performance reasons, in 637a096 I reverted the ProgramConfiguration#setUniforms → ProgramConfiguration#getUniforms switch, but rather than calling gl.uniform** setters directly from the binders they have UniformBinding members. This allows us to save time across e.g. different tiles within the same layer in a frame, and also allows us to reuse those across different frames iff the ProgramConfiguration hasn't been used in a different Program and vice versa (bound uniforms remain bound to those locations on a given program even when the active program is changed). The current benchmarks (also updated in OP) are https://bl.ocks.org/anonymous/raw/d3873066e1fe5d97d9c0db5879c0c66d/ : some of the individual layer benchmarks show slight degradation, but the aggregate Paint benchmark is now stable and I think the real gains here are in map styles with multiple layer types.

jfirebaugh · 2018-03-10T02:32:59Z

src/render/program.js

@@ -90,7 +90,6 @@ class Program<Us: UniformBindings> {
            }
        }

-        this.binderUniforms = configuration.getUniformBindings(context);


This should still live on Program, so that sets are deduped across different ProgramConfigurations.

jfirebaugh · 2018-03-16T17:40:19Z

src/render/draw_line.js

        firstTile = false;
+        // once refactored so that bound texture state is managed, we'll also be able to remove this firstTile/programChanged logic


Looks like the specific issue here is that painter.lineAtlas.bind(context) must currently be called after lineSDFUniformValues(...), because the latter produces side effects in the lineAtlas when it calls lineAtlas.getDash(...).

lbud · 2018-03-17T00:18:18Z

Before 8230e38: https://bl.ocks.org/anonymous/raw/7f4b0baa728fc39995ab1c13a267cdef/
After: https://bl.ocks.org/anonymous/raw/51cc28f88a2eb55da5e8a3b03dac3f05/ — a few individual layer benchmarks are still slightly slower, but less so than before…

lbud · 2018-03-21T22:13:52Z

e5b91bd improves upon layer performance by switching ProgramConfiguration#binders from an object to an array, so that setUniforms can iterate over an array (with pre-cached length) rather than hash properties (jsperf experiment). Current benchmarks: http://bl.ocks.org/lbud/raw/f7e5f62c79d77f5c554ed8a4978c61d8/ (LayerLine floats back and forth around master performance, sometimes slightly better and sometimes slightly worse, but consistently better than before this commit.)

lbud · 2018-06-18T21:58:44Z

Benchmarks for current rebased version: http://bl.ocks.org/lbud/raw/9920556dde95d6b156e838559b36596d/

There are a few layer-specific benchmarks that are still slower. The paint benchmark is consistently performing as well if not slightly better than 0.45 + master.

I can't reproduce the disparity within LayerCircle, LayerSymbol, or StyleLayerCreate during individual runs
LayerBackground and LayerLine are consistently performing slightly slower. I'm inclined to not worry about a background benchmark as maps typically have only one or, at most, a few background layers

anandthakker · 2018-08-06T14:34:33Z

Re-rebased. LayerBackground is still about 100 microseconds slower, but I agree with @lbud that this is acceptable for background layers. LayerLine is also still about 60 microseconds-- ~1.5 standard deviations -- slower. But given that Paint shows no significant difference, I'm inclined to say we should go ahead with this.

* Introduce uniform bining classes * Set uniforms and color/depth/stencil modes within Program#draw * Use drawElements exclusively (eliminate all uses of drawArrays)

… initialize bindings in program constructor

jfirebaugh

🎉

lbud requested a review from anandthakker January 18, 2018 22:42

lbud force-pushed the state-uniforms branch from 89416b4 to d79288f Compare January 19, 2018 23:41

anandthakker reviewed Jan 26, 2018

View reviewed changes

lbud force-pushed the state-uniforms branch 6 times, most recently from df5fbd9 to c9049a9 Compare February 28, 2018 00:12

jfirebaugh reviewed Feb 28, 2018

View reviewed changes

lbud force-pushed the state-uniforms branch 6 times, most recently from 7988f3d to 637a096 Compare March 7, 2018 00:44

jfirebaugh reviewed Mar 10, 2018

View reviewed changes

lbud force-pushed the state-uniforms branch from 637a096 to 4f34b0e Compare March 15, 2018 23:45

lbud requested a review from jfirebaugh March 16, 2018 00:45

jfirebaugh reviewed Mar 16, 2018

View reviewed changes

lbud force-pushed the state-uniforms branch 4 times, most recently from 6d03fa5 to e5b91bd Compare March 21, 2018 22:09

lbud force-pushed the state-uniforms branch 2 times, most recently from fa68809 to e019157 Compare June 14, 2018 21:36

lbud force-pushed the state-uniforms branch from e019157 to 147deca Compare June 14, 2018 23:50

lbud force-pushed the state-uniforms branch from 147deca to 2121a68 Compare June 18, 2018 21:58

anandthakker force-pushed the state-uniforms branch from 2121a68 to d710415 Compare August 6, 2018 13:28

anandthakker force-pushed the state-uniforms branch 3 times, most recently from c34e74d to 3042bd2 Compare August 8, 2018 12:43

Lauren Budorick and others added 11 commits August 8, 2018 08:57

Introduce state management for uniform bindings

63b7b02

* Introduce uniform bining classes * Set uniforms and color/depth/stencil modes within Program#draw * Use drawElements exclusively (eliminate all uses of drawArrays)

Maintain ownership of binder uniforms on Programs

dba3231

Perf improvements: initalize location in uniform binding constructor,…

3a6a2f3

… initialize bindings in program constructor

Check for most commonly divergent matrix indices first

ebb5576

Inline Uniforms class

dd5d5c8

Inline drawLineTile and hoist loop invariant code

8c8f9f8

Set GL program state inside Program#draw

4541c22

Break cyclic dependency between Program and ProgramConfiguration

0791813

Cache gl property

ae87c35

Fix line drawing

1a9cfd7

Drop v suffix from Uniform* names

4c843a7

anandthakker force-pushed the state-uniforms branch from 3042bd2 to 4c843a7 Compare August 8, 2018 12:57

jfirebaugh approved these changes Aug 8, 2018

View reviewed changes

jfirebaugh merged commit 855b850 into master Aug 8, 2018

jfirebaugh deleted the state-uniforms branch August 8, 2018 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce uniform bindings #6018

Introduce uniform bindings #6018

lbud commented Jan 18, 2018 •

edited

Loading

anandthakker left a comment

anandthakker Jan 24, 2018

anandthakker Jan 24, 2018

lbud Feb 22, 2018

jfirebaugh Feb 28, 2018

anandthakker Jan 26, 2018

lbud Feb 22, 2018

anandthakker Jan 26, 2018

lbud commented Feb 28, 2018

jfirebaugh Feb 28, 2018

jfirebaugh Feb 28, 2018

jfirebaugh Feb 28, 2018

lbud commented Mar 7, 2018

jfirebaugh Mar 10, 2018

jfirebaugh Mar 16, 2018

lbud commented Mar 17, 2018 •

edited

Loading

lbud commented Mar 21, 2018 •

edited

Loading

lbud commented Jun 18, 2018

anandthakker commented Aug 6, 2018

jfirebaugh left a comment


		import type Context from '../gl/context';

		export interface UniformInterface<T> {

		firstTile = false;
		// once refactored so that bound texture state is managed, we'll also be able to remove this firstTile/programChanged logic

Introduce uniform bindings #6018

Introduce uniform bindings #6018

Conversation

lbud commented Jan 18, 2018 • edited Loading

Launch Checklist

anandthakker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbud commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbud commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbud commented Mar 17, 2018 • edited Loading

lbud commented Mar 21, 2018 • edited Loading

lbud commented Jun 18, 2018

anandthakker commented Aug 6, 2018

jfirebaugh left a comment

Choose a reason for hiding this comment

lbud commented Jan 18, 2018 •

edited

Loading

lbud commented Mar 17, 2018 •

edited

Loading

lbud commented Mar 21, 2018 •

edited

Loading