Refactored bucket classes [do not merge]

[lucaswoj]

cc @jfirebaugh @mourner @tmcw @ansis

The overall goal of this PR is to clean up the Bucket classes and refactor them to lay the groundwork for per-layer buffer layouts. This PR includes the following changes

• renamed Bucket to BufferBuilder
• added some utility methods to BufferBuilder to cut down on boilerplate and repeated code
• simplified BufferBuilder creation (remove createBucket module, remove external property initialization by WorkerTile)
• added LayerType objects
• removed BufferSet and made BufferBuilder responsible for creating its own buffers
• changed Buffer::push to accept an object

Performance

How's performance? It's not great / not bad. I think we can improve it further.

# bucket2

duration: 30000
3001 ms, 120 samples for /dist/mapbox-gl.js
4318 ms, 102 samples for /dist/mapbox-gl.js

# master
duration: 30000
2716 ms, 123 samples for /dist/mapbox-gl.js
3824 ms, 108 samples for /dist/mapbox-gl.js

Next Steps

• documentation
• testing
• performance tuning

[lucaswoj]

Sorry about undoing your work here @mourner . We need to construct attribute from scratch because it sometimes includes some extraneous non-serializable fields (namely the value property which is a function)

[mourner]

Yeah, this looks good. One minor remark is that asserting itemSize should probably happen after it gets its new value.

[lucaswoj]

Good catch.

[lucaswoj]

I might've gotten the benchmarks wrong before. Here's what I'm seeing now:

duration: 30000
3145 ms, 116 samples for http://localhost:9000/bucket2.js
2710 ms, 120 samples for http://localhost:9000/master.js
2674 ms, 121 samples for http://localhost:9000/master.js
4763 ms, 102 samples for http://localhost:9000/bucket2.js

[mourner]

That doesn't look very good, we'll have to find a way to cut this down...

[lucaswoj]

Agreed. I'm working on it.

• remove all uses of call and apply
• simplify BufferBuilder as much as possible
• add more eval magic 😈
• use a different & more efficient code path for attributes that won't be disabled (i.e. pos and data)

[lucaswoj]

With a little bit of 😈 we can get

duration: 30000
2022 ms, 126 samples for /dist/mapbox-gl.js
1721 ms, 126 samples for http://localhost:9000/master.js
2275 ms, 121 samples for /dist/mapbox-gl.js
1635 ms, 126 samples for http://localhost:9000/master.js
2041 ms, 123 samples for /dist/mapbox-gl.js

[lucaswoj]

I think we can squeeze out a little more perf by looking into

use a different & more efficient code path for attributes that won't be disabled (i.e. pos and data)

... but I'm not sure if its worth the effort / additional complexity.

What are your thoughts about the latest numbers, @mourner @jfirebaugh? About the overall architecture?

[lucaswoj]

• use a different & more efficient code path for attributes that won't be disabled (i.e. pos and data)

Went down this 🐰 hole for a few hours this morning and couldn't find the right arrangement to actually make this faster (while still being comprehensible to human beings).

Working in this hot code is hard. Adding a single array/object allocation or function call can be VERY expensive.

[mourner]

I feel you. :( Awesome progress though! And very very evil 😈

[lucaswoj]

I understand this looks like a lot of code, but the only new logic is in BufferBuilder. This class is a much like the Bucket class but with some new abstractions and API niceties.

The logic in the old Bucket classes has moved, unchanged, to the LayerType objects and BufferBuilder subclasses.

Most other files have only changed to accommodate little differences between the Bucket and BufferBuilder APIs.

[lucaswoj]

Unfortunately, its a big 700-LOC 💊 to swallow and I can't identify any clean way to break this into multiple PRs. I am happy to walk anyone through the changes here over voice or answer any questions.

With this PR, I feel like I can see the light at the end of the data-driven styling tunnel (partially obscured by #1336 😬)

[lucaswoj]

I think this is in a good place now for 👀 @jfirebaugh

[lucaswoj]

In future non-urgent PRs, I think it'd be nice to

• move shader uniform calculations to LayerType & get rid of premultiplyLayer
• move shader definitions from Painter to LayerType
• move draw_* functions to LayerType
• move buffer building to LayerType (requires making BufferBuilder stateless)
• set the gl draw type (e.g. gl.TRIANGLES) in LayerType and infer element buffer layout

[lucaswoj]

I'd like to eventually move in the direction of b49a929 but that'll require enough refactoring (namely for SymbolBucket) that I think it deserves a separate PR.

[lucaswoj]

I'm going to shuffle some things around in here to nudge this towards the architecture in mapbox/mapbox-gl-native#2730

• turn stateless LayerType classes into subclasses of StyleLayer/Layer
• get rid of buffer builder subclasses, do all building in new Layer subclasses

I might also take this as an opportunity to recut this big PR into a couple smaller PRs.

[lucaswoj]

Closed re previous comment