Refactored bucket classes [do not merge] by lucaswoj · Pull Request #1620 · mapbox/mapbox-gl-js

lucaswoj · 2015-10-13T23:39:13Z

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 · 2015-10-13T23:40:15Z

js/data/buffer.js

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)

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

Good catch.

lucaswoj · 2015-10-14T18:33:34Z

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 · 2015-10-14T18:53:56Z

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

lucaswoj · 2015-10-14T20:39:03Z

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 · 2015-10-14T23:21:47Z

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 · 2015-10-15T00:22:23Z

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 · 2015-10-15T18:15:06Z

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 · 2015-10-15T18:17:31Z

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

lucaswoj · 2015-10-15T22:01:08Z

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 · 2015-10-15T22:03:54Z

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 · 2015-10-16T00:04:40Z

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

lucaswoj · 2015-10-16T19:12:20Z

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

Also moved some constructor options to SymbolBufferBuilder

lucaswoj · 2015-10-20T17:24:51Z

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 · 2015-10-22T19:15:03Z

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 · 2015-10-23T17:28:38Z

Closed re previous comment

lucaswoj reviewed Oct 13, 2015
View reviewed changes

lucaswoj force-pushed the bucket2 branch from 6cb1786 to f3ad3a1 Compare October 14, 2015 18:31

lucaswoj force-pushed the bucket2 branch from e922266 to 6cb9bcd Compare October 14, 2015 23:56

lucaswoj force-pushed the bucket2 branch from 4531a97 to 66b5c8f Compare October 15, 2015 23:29

lucaswoj changed the title ~~Refactored bucket classes [DO NOT MERGE]~~ Refactored bucket classes Oct 16, 2015

This was referenced Oct 16, 2015

Write StyleLayer2 Class #1580

Closed

Data-Driven Styling Master Ticket #1626

Closed

Lucas Wojciechowski added 10 commits October 19, 2015 13:10

Added Bucket2 class, migrated circle

f29bb9d

Use layer type uniform values

b11f639

Removed custom push methods

0f60afa

Added special push object to layer type iterator

efe428b

Automatically handle element group lengths

ab28da2

Added better api

f53e106

Converted fill

28161eb

Convertex line buckets

1132909

Fixed circle uniforms

f93438d

Rewrote symbol bucket

99b4aaf

Lucas Wojciechowski added 19 commits October 19, 2015 13:10

Use array instead of an object in Buffer::push

d7db04a

Fixed incorrect makeRoomFor calls

a14456d

😈

afd728d

Simplified createElementAddMethod

4b8987d

Remove BufferBuilder class methods

85e5689

Refactoring

92f87ef

Refactoring

dfdc2b6

Skip _createPushMethod in main thread

55cc147

Only call _refreshViews in worker thread

128d67f

Improved documentation on Buffer methods

0a3bba0

Minor refactoring

790ee8d

Made BufferBuilder::type set by subclasses

6c352bf

Removed fill layer type uniforms

5519090

Added basic BufferBuilder tests

ccb1620

Added documentation to BufferBuilder

2ef457f

Also moved some constructor options to SymbolBufferBuilder

Remove all references to "bucket"

c6cda4b

PR cleanup

d27f159

Revert bench changes

1cf8ae8

Revert bucket-stats changes

b5512a7

lucaswoj force-pushed the bucket2 branch from 5411d06 to b5512a7 Compare October 19, 2015 20:14

lucaswoj force-pushed the bucket2 branch from e7bd64e to 5e2a0d7 Compare October 21, 2015 17:15

Lucas Wojciechowski added 2 commits October 21, 2015 10:19

Fixed typo in line_buffer_builder

6e9bca9

Fix reloadSymbolData error

3e49540

lucaswoj force-pushed the bucket2 branch from 5e2a0d7 to 3e49540 Compare October 21, 2015 17:19

lucaswoj changed the title ~~Refactored bucket classes~~ Refactored bucket classes [do not merge] Oct 22, 2015

lucaswoj closed this Oct 23, 2015

lucaswoj deleted the bucket2 branch November 2, 2015 22:00

Conversation

lucaswoj commented Oct 13, 2015

Performance

Next Steps

Uh oh!

lucaswoj Oct 13, 2015

Choose a reason for hiding this comment

Uh oh!

mourner Oct 14, 2015

Choose a reason for hiding this comment

Uh oh!

lucaswoj Oct 14, 2015

Choose a reason for hiding this comment

Uh oh!

lucaswoj commented Oct 14, 2015

Uh oh!

mourner commented Oct 14, 2015

Uh oh!

lucaswoj commented Oct 14, 2015

Uh oh!

lucaswoj commented Oct 14, 2015

Uh oh!

lucaswoj commented Oct 15, 2015

Uh oh!

lucaswoj commented Oct 15, 2015

Uh oh!

mourner commented Oct 15, 2015

Uh oh!

lucaswoj commented Oct 15, 2015

Uh oh!

lucaswoj commented Oct 15, 2015

Uh oh!

lucaswoj commented Oct 16, 2015

Uh oh!

lucaswoj commented Oct 16, 2015

Uh oh!

lucaswoj commented Oct 20, 2015

Uh oh!

lucaswoj commented Oct 22, 2015

Uh oh!

lucaswoj commented Oct 23, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants