Added Buffer2 class by lucaswoj · Pull Request #1526 · mapbox/mapbox-gl-js

lucaswoj · 2015-09-22T23:48:23Z

recut from #1414

Up until now, GL JS has used hand-coded classes to specify buffer layouts and buffer manipulation operations. This worked well pre-data-driven-styling because there were < 10 possible buffer layouts. For example, here is the TriangleElementBuffer class

module.exports = TriangleElementBuffer;

function TriangleElementBuffer(buffer) {
    Buffer.call(this, buffer);
}

TriangleElementBuffer.prototype = util.inherit(Buffer, {
    itemSize: 6, // bytes per triangle (3 * unsigned short == 6 bytes)
    arrayType: 'ELEMENT_ARRAY_BUFFER',

    add: function(a, b, c) {
        var pos2 = this.pos / 2;

        this.resize();

        this.ushorts[pos2 + 0] = a;
        this.ushorts[pos2 + 1] = b;
        this.ushorts[pos2 + 2] = c;

        this.pos += this.itemSize;
    }
});

Post-data-driven styling, however, the number of possible buffer layouts is practically infinite because any subset of paint properties might be included in the buffer.

The first step towards proper data driven styling was to create a more general and powerful framework for buffers (temporarily called Buffer2) which allows buffer layouts to be specified by configuration rather than code. Using Buffer2, We could write the same TriangleElementBuffer class as

module.exports = function() {
    Buffer2.apply(this, {
        type: Buffer2.BufferType.ELEMENT,
        attributes: {
            verticies: {
                components: 3,
                type: Buffer2.AttributeType.UNSIGNED_SHORT
            }
        }
    });
}

... but in practice we wouldn't write it that way. GL JS would choose the attributes for that buffer at runtime for the best performance.

The Buffer2 class will know how to construct, write to, and read from GL buffers but have no semantic understanding of their contents.

Instances of Buffer2 can be (destructively) serialized and transferred across threads.

cc @tmcw @mourner @kkaefer

mourner · 2015-09-23T11:02:02Z

buffer_shim stuff is temporary until you move over everything to Buffer2, right?

mcwhittemore · 2015-09-23T16:16:48Z

js/data/buffer2.js

@lucaswoj wanna move this to the top of the function so we don't do the above work if we're just going to throw an error.

well, if it we're actually throwing an error, performance doesn't matter much :)

true, still, better to do validation before the work not after, if only for readability :)

This is an overloaded constructor. Like Buffer1, it can either

create an empty buffer from a configuration object

rehydrate a serialized instance of Buffer

The idea with the util.assert(this.type) line is that we allow each case to parse the constructor arguments independently and then perform a little validation on the resulting instance. It is not necessary to perform this assertion but I found that this.type had a tendency to end up as undefined and that resulted in hard-to-debug errors.

In a larger sense, I sorta want to rewrite this whole constructor to eliminate this branching altogether. It should be possible to combine these two cases into one.

I'd also consider removing asserts once the code has been tested and finalized. It's kinda all or nothing — either we add asserts everywhere throughout the whole code bringing a lot of cruft with it, or don't add them and rely on tests and throwing specific errors in special cases instead.

lucaswoj · 2015-09-23T18:39:35Z

@mourner Yes, the "shim" is a temporary measure to DRY up the code that wraps a Buffer2 in an old-style "Buffer1" per your suggestion in #1414 (comment).

My intention is for the shim to get merged into master temporarily so we can start testing Buffer2 in the real world before we finish rewriting everything that touches Buffer1.

mourner · 2015-09-23T18:45:14Z

Are they incompatible enough to write a shim? I see that it proxies most of the methods. Could you just make Buffer2 have things that other code relies on in Buffer1, and then remove any cruft after rewriting that code?

lucaswoj · 2015-09-23T23:30:46Z

The latest commit

removes the shim in favor of direct compatibility with Buffer1
removes the special serialization logic (which was both unnecessary and a huge pain to support)
implements TriangleElementBuffer as a subclass of Buffer2
has some fill layer rendering errors

mourner · 2015-09-24T07:08:58Z

js/data/buffer/triangle_element_buffer.js

could those move to the prototype below?

Yes, the attributes could move to the prototype, but ideally they wouldn't move to the prototype. The raison-d'etre of Buffer2 is that it has a configurable buffer layout (vs Buffer1 which had a hardcoded buffer layout). Layout is intended to be passed as a constructor argument at runtime. This code represents an intermediary step towards that goal.

mourner · 2015-09-24T07:10:47Z

@lucaswoj looks great! Let's figure out the fill issues before proceeding.

mourner · 2015-09-24T07:16:42Z

js/util/util.js

Could we just use console.assert instead of a custom function here? In addition to making it unnecessary, this has an advantage of being supported by https://github.com/twada/unassertify

Looks like console.assert is not and will not be widely supported. util.assert is just a shorthand around if (blah) throw new Error(...). Validating preconditions is especially helpful because WebGL is bad at reporting errors. I can remove the util.assert statements before merging this code if you prefer.

No, I'm convinced on assertions — let's keep, but looks like we'll use the assert module so that it works with unassertify.

Just merged #1557, so if you rebase on master you can start using assert.

lucaswoj · 2015-09-24T18:47:59Z

Latest commit addresses PR feedback, renames Buffer2::size to Buffer2::capacity (a la c++ vectors) and fixes rendering issues.

lucaswoj · 2015-09-24T18:57:46Z

FPS benchmarks look 👍 so far

duration: 3000
29 fps https://api.tiles.mapbox.com/mapbox-gl-js/v0.10.0/mapbox-gl.js
29 fps https://api.tiles.mapbox.com/mapbox-gl-js/v0.11.0/mapbox-gl.js
29.2 fps /debug/mapbox-gl.js
28.7 fps https://api.tiles.mapbox.com/mapbox-gl-js/v0.10.0/mapbox-gl.js
28.9 fps https://api.tiles.mapbox.com/mapbox-gl-js/v0.11.0/mapbox-gl.js
29 fps /debug/mapbox-gl.js

jfirebaugh · 2015-09-25T16:47:08Z

I would expect performance effects to be largely confined to the Worker. The end result is the same on the GPU, so FPS wouldn't change, but the time taken to build the Buffer in the background could change. It would be nice to have some benchmarks for that.

lucaswoj · 2015-09-25T16:55:57Z

@jfirebaugh How about we add a benchmark that measures the total time spent creating buffers during a slow flyTo from DC to SF? Ticketed at #1559

lucaswoj · 2015-09-25T19:47:30Z

A very unscientific test using the new bufferstats branch seems to indicate that Buffer2 does not add substantial overhead to buffer creation.

lucaswoj · 2015-09-28T23:41:35Z

Thanks to @mcwhittemore, we now have all buffers rewritten using Buffer2 and...

drumroll...

Its about 10x slower than master

## buffer2 
duration: 30000
9140 ms, 46 samples for /debug/mapbox-gl.js
10198 ms, 45 samples for /debug/mapbox-gl.js

# master
duration: 30000
1225 ms, 112 samples for /debug/mapbox-gl.js
1235 ms, 119 samples for /debug/mapbox-gl.js

I haven't even begun to think about performance optimizations, so I'm hopeful that there'll be some easy gains.

lucaswoj · 2015-09-29T19:27:41Z

Got the bucket-stats benchmark down to ~3500 ms in the latest push. I'm not seeing any more low hanging fruit right now.

3654 ms, 111 samples for /debug/mapbox-gl.js
3416 ms, 116 samples for /debug/mapbox-gl.js

This is roughly 2x slower than Buffer1

mourner · 2015-09-29T20:18:43Z

Just as I predicted a while back. I could take a look at optimizing this more.

In the worst case, if we can't make any more perf improvements and it's still too slow, there's an option of code generation, similar to what I did with layer filters. It can bring us back to master performance while being dynamically generated from attribute config.

jfirebaugh · 2015-09-29T20:29:44Z

js/data/buffer.js

Performance optimization: eliminate this condition. Either:

Always require an array, or

Write this.set = function() { ... } dynamically in the constructor, and make it require an array for multiple attributes, and require no array for single attributes. For multiple attributes, it may help to iterate over options.attributes rather than item (like, maybe the compiler will detect that options.attributes is constant).

Writing this.set = function() { ... } dynamically in the constructor can hopefully give you some nice speed ups, while stopping just short of needing code-gen + eval.

lucaswoj · 2015-09-29T23:17:01Z

js/data/buffer/line_vertex_buffer.js

LineVertexBuffer::add is one of the hottest functions in the codebase. It looks like dropping this call to Math.round gives us a substantial performance boost but changes the output just enough to break test-suite. Can we afford to modify test-suite for this? Can we leverage this information in another way? cc @mourner @jfirebaugh

cc @kkaefer -- the rounding here goes way back, at least as far as 5cf0c0b#diff-ede55fc20965ad828d7e5cf6eaa5c404R50.

lucaswoj · 2015-09-30T00:15:39Z

Just wrote up a little code-gen prototype. With the removal of Math.round thrown in, the buffer2 branch is neck and neck with master!

1751 ms, 127 samples for /debug/mapbox-gl.js
1846 ms, 123 samples for http://localhost:9000/mapbox-gl-master.js
2440 ms, 115 samples for /debug/mapbox-gl.js
2225 ms, 120 samples for http://localhost:9000/mapbox-gl-master.js

... without the removal of Math.round, things don't look so great

duration: 30000
2355 ms, 122 samples for /debug/mapbox-gl.js
1856 ms, 130 samples for http://localhost:9000/mapbox-gl-master.js
3302 ms, 115 samples for /debug/mapbox-gl.js
1999 ms, 124 samples for http://localhost:9000/mapbox-gl-master.js

I've openend a separate ticket for this discussion at #1573

mourner · 2015-09-30T06:41:24Z

@lucaswoj if the methods are generated now, why is it slower with Math.round if it was present before? Performance should be the same if we generate the same code.

mourner · 2015-09-30T10:23:56Z

One of the things that could cause slowdowns is the need to create new objects and arrays for every push. Now we have the following syntax:

this.push({
    pos: [
        (point.x << 1) | tx,
        (point.y << 1) | ty
    ],
    data: [
        EXTRUDE_SCALE * extrude.x,
        EXTRUDE_SCALE * extrude.y,
        Math.round(EXTRUDE_SCALE * extrude.x),
        Math.round(EXTRUDE_SCALE * extrude.y)
    ]
});

Can we change the generated code to turn this into e.g. just the following?

this.push(
    (point.x << 1) | tx, 
    (point.y << 1) | ty,
    EXTRUDE_SCALE * extrude.x,
    EXTRUDE_SCALE * extrude.y,
    Math.round(EXTRUDE_SCALE * extrude.x),
    Math.round(EXTRUDE_SCALE * extrude.y));

mourner · 2015-09-30T14:05:37Z

Yep, that seems to do it. Sketched out the idea in bea8d6d. On my machine, the bench produces:

539 ms, 128 samples for /dist/mapbox-gl.js # master
741 ms, 128 samples for /dist/mapbox-gl.js # buffer2
566 ms, 128 samples for /dist/mapbox-gl.js # flat-push
558 ms, 129 samples for /dist/mapbox-gl.js # flat-push, no Math.round

For some reason removing Math.round doesn't affect my results much, but this may be machine-specific.

Also note how the buffer code could be simplified further with this flat approach.

mourner · 2015-09-30T14:40:04Z

On a side note, just noticed that glyph_vertex_buffer and icon_vertex_buffer are pretty much identical. We can remove one of them.

mourner · 2015-09-30T15:19:02Z

js/data/buffer/circle_vertex_buffer.js

This should be SHORT rather than UNSIGNED_SHORT.

mourner · 2015-09-30T15:57:58Z

Also, line_element_buffer is identical to triangle_element_buffer so can be removed as well.

Generally I see that we're in a great position to remove *_buffer.js classes in this PR and turn them into sets of attribute configs like @lucaswoj envisioned (even before writing Layer). For that to happen, we need to eliminate custom add and bind methods.

To eliminate add (that's already identical to push in my branch for many buffer types), we simply move the layer-specific logic to the corresponding bucket implementations, so this:

add: function(x, y, extrudeX, extrudeY) {
    this.push(
        (x * 2) + ((extrudeX + 1) / 2),
        (y * 2) + ((extrudeY + 1) / 2));
},
...
circleVertex.add(x, y, -1, -1);
circleVertex.add(x, y, 1, -1);

Becomes:

circleVertex.push(x * 2, y * 2);
circleVertex.push(x * 2 + 1, y * 2);

We also may want to keep the multiply-floor packing logic in attribute configs so that things like this are not a bucket concern:

this.push(
    x, y,
    Math.floor(ox * 64), // use 1/64 pixels for placement
    Math.floor(oy * 64),
    Math.floor(tx / 4), /* tex */
    Math.floor(ty / 4), /* tex */

To do this, we simply add a multiplier option to attributes so this becomes:

this.push(
    x, y,
    ox, oy,
    tx, ty,
...
attributes: [{
    name: 'pos',
    components: 2,
    type: Buffer.AttributeType.SHORT
}, {
    name: 'extrude',
    components: 2,
    type: Buffer.AttributeType.SHORT,
    multiplier: 64, // use 1/64 pixels for placement
}, {
    name: 'data1',
    components: 4,
    type: Buffer.AttributeType.UNSIGNED_BYTE,
    multiplier: 1 / 4
}, {

I'm up for dropping Math.round everywhere because it doesn't affect rendering much and floor is implied when updating the views so this makes things simpler and faster.

To eliminate custom bind, we can simply write a generic implementation that uses the attributes configuration of the buffer. Currently this kind of code is redundant:

gl.vertexAttribPointer(shader.a_pos, 2, gl.SHORT, false, stride, offset + 0);
gl.vertexAttribPointer(shader.a_offset, 2, gl.SHORT, false, stride, offset + 4);
gl.vertexAttribPointer(shader.a_data1, 4, gl.UNSIGNED_BYTE, false, stride, offset + 8);
gl.vertexAttribPointer(shader.a_data2, 2, gl.UNSIGNED_BYTE, false, stride, offset + 12);

To make things more confusing, vertexAttribPointer configuration happens in two places. For some buffer types like line vertex, it's inside draw_*.js, and for others like icon vertex it's in *_buffer.js. So writing a generic bind in buffer.js will make the code much cleaner.

mourner · 2015-09-30T16:41:12Z

Feel free to fast-forward flat-push branch into this one if the changes make sense.

lucaswoj · 2015-09-30T17:58:20Z

Thanks for the 👀 @mourner! I appreciate your thoughtful feedback. Below, I've enumerated your proposed next steps and added my own thoughts.

add a `multiplier` option to attributes

I agree that this multiplier should be pulled out into configuration 👍. However, I'm envisioning it living on the StyleLayer2 class, where it can be shared between buffer attribute generation and constant attribute generation (i.e. with and without disableVertexAttribArray). I propose we leave it inlined with calls to add for now and pull it out in the StyleLayer2 PR.

merge `flat-push` branch

Sad to lose the self-documenting object keys but this will make hot code faster so we should do it. ✅

remove all `*_buffer` classes

add a universal bind method
remove add method

The plan was to land this in a separate PR but it looks like a small undertaking with a big performance bump, so we should do it! ✅

drop `Math.round` in all buffers

Let's do this one in a separate PR. It is conceptually unrelated and might generate some discussion. ref #1573 ✅

merge `line_element_buffer` and `triangle_element_buffer` classes

This will be subsumed by "remove all *_buffer classes"

mourner · 2015-09-30T18:27:15Z

@lucaswoj good plan, let's move with this!

The plan was to land this in a separate PR but it looks like a small undertaking with a big performance bump, so we should do it!

Actually I think we can leave this for a separate PR. It probably doesn't affect performance too much, I was looking forward to it mostly because it removes unnecessary code and keeps all buffer logic in one neat place.

merge line_element_buffer and triangle_element_buffer classes

Also icon_vertex_buffer vs glyph_vertex_buffer.

lucaswoj · 2015-09-30T19:26:01Z

I think this is ready to 🚢! Any last words?

duration: 30000
1649 ms, 116 samples for /debug/mapbox-gl.js
1678 ms, 124 samples for http://localhost:9000/mapbox-gl-master.js
2846 ms, 106 samples for /debug/mapbox-gl.js
2210 ms, 121 samples for http://localhost:9000/mapbox-gl-master.js

cc @mourner @jfirebaugh @mcwhittemore

mcwhittemore · 2015-09-30T19:34:02Z

js/data/buffer.js

NVM. Offset is what's changing here.

lucaswoj · 2015-09-30T22:27:27Z

Going to 🚢 as soon as CI passes

mourner · 2015-10-01T06:38:36Z

Not sure why you squashed everything into one commit, both me and John where not in favor of that...

mourner · 2015-10-01T07:17:56Z

Oh, sorry, I see the problem now — it was hard to rebase in a way where each commit is in a good state, so squashing is good here. Fair enough.

lucaswoj · 2015-10-01T16:17:35Z

Sorry about the squash, @mourner. I tried to come up with a clean rebase but there were a bugs and accidental check-ins in the intermediate commits.

mcwhittemore reviewed Sep 23, 2015
View reviewed changes

lucaswoj force-pushed the buffer2 branch from 6470103 to 0c01274 Compare September 23, 2015 23:26

lucaswoj force-pushed the buffer2 branch 2 times, most recently from da3266c to fbe0a60 Compare September 24, 2015 00:12

mourner reviewed Sep 24, 2015
View reviewed changes

lucaswoj force-pushed the buffer2 branch from fbe0a60 to 269f49b Compare September 24, 2015 18:47

lucaswoj mentioned this pull request Sep 25, 2015

Add a "buffer creation time" benchmarking script #1559

Closed

lucaswoj force-pushed the buffer2 branch from 269f49b to ae898fb Compare September 25, 2015 17:36

lucaswoj force-pushed the buffer2 branch 2 times, most recently from d714667 to 5287956 Compare September 28, 2015 23:14

lucaswoj mentioned this pull request Sep 29, 2015

when zooming out, map flashes blank #1570

Closed

jfirebaugh reviewed Sep 29, 2015
View reviewed changes

lucaswoj force-pushed the buffer2 branch from fca6628 to ada7893 Compare September 29, 2015 23:03

lucaswoj reviewed Sep 29, 2015
View reviewed changes

lucaswoj mentioned this pull request Sep 30, 2015

Remove Math.round in LineVertexBuffer #1573

Closed

mourner reviewed Sep 30, 2015
View reviewed changes

js/data/buffer/circle_vertex_buffer.js Outdated

Copy link

Member

mourner Sep 30, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be SHORT rather than UNSIGNED_SHORT.

mcwhittemore reviewed Sep 30, 2015
View reviewed changes

js/data/buffer.js Outdated

Copy link

Contributor

mcwhittemore Sep 30, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NVM. Offset is what's changing here.

lucaswoj force-pushed the buffer2 branch from 292d443 to 5071189 Compare September 30, 2015 22:17

Made Buffer class more powerful and runtime configurable

f0faa3a

lucaswoj force-pushed the buffer2 branch from 5071189 to f0faa3a Compare September 30, 2015 22:22

lucaswoj merged commit f0faa3a into master Sep 30, 2015

lucaswoj deleted the buffer2 branch September 30, 2015 22:31

mourner mentioned this pull request Oct 1, 2015

Buffer2 follow-up refactoring (remove *_buffer classes) #1578

Merged

This was referenced Oct 16, 2015

Data-Driven Styling Master Ticket #1626

Closed

Add buffer benchmark warmup & cooldown #1634

Merged

Conversation

lucaswoj commented Sep 22, 2015

Uh oh!

mourner commented Sep 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucaswoj commented Sep 23, 2015

Uh oh!

mourner commented Sep 23, 2015

Uh oh!

lucaswoj commented Sep 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mourner commented Sep 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucaswoj commented Sep 24, 2015

Uh oh!

lucaswoj commented Sep 24, 2015

Uh oh!

jfirebaugh commented Sep 25, 2015

Uh oh!

lucaswoj commented Sep 25, 2015

Uh oh!

lucaswoj commented Sep 25, 2015

Uh oh!

lucaswoj commented Sep 28, 2015

Uh oh!

lucaswoj commented Sep 29, 2015

Uh oh!

mourner commented Sep 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucaswoj commented Sep 30, 2015

Uh oh!

mourner commented Sep 30, 2015

Uh oh!

mourner commented Sep 30, 2015

Uh oh!

mourner commented Sep 30, 2015

Uh oh!

mourner commented Sep 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mourner commented Sep 30, 2015

Uh oh!

mourner commented Sep 30, 2015

Uh oh!

lucaswoj commented Sep 30, 2015

add a multiplier option to attributes

merge flat-push branch

remove all *_buffer classes

drop Math.round in all buffers

add a `multiplier` option to attributes

merge `flat-push` branch

remove all `*_buffer` classes

drop `Math.round` in all buffers

merge `line_element_buffer` and `triangle_element_buffer` classes