Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Yet another Bucket refactor!? #5137

Merged
merged 4 commits into from
Aug 14, 2017
Merged

Yet another Bucket refactor!? #5137

merged 4 commits into from
Aug 14, 2017

Conversation

jfirebaugh
Copy link
Contributor

@jfirebaugh jfirebaugh commented Aug 11, 2017

Yeah, I know, I've been refactoring Bucket for years. But it needs it!

This series of refactors culminates in the elimination of ArrayGroup and BufferGroup. These classes were a poor abstraction: they required highly conditional logic to satisfy the needs of all their consumers, and they were tightly coupled to each other. It's better to have each bucket class contain exactly the array/buffer properties it needs, and to keep corresponding arrays and buffers close to each other in code.

This brings the bucket classes much closer to their corresponding gl-native implementations.

benchmark master 4eb5d98 bucket-interface 7998c6d
map-load 121 ms 111 ms
style-load 64 ms 58 ms
buffer 983 ms 976 ms
fps 59 fps 60 fps
frame-duration 5.8 ms, 0% > 16ms 5.9 ms, 0% > 16ms
query-point 0.76 ms 0.73 ms
query-box 41.95 ms 42.29 ms
geojson-setdata-small 3 ms 2 ms
geojson-setdata-large 93 ms 109 ms

@mourner
Copy link
Member

mourner commented Aug 11, 2017

Weird, the Circle build log keeps stuck in loading for me...

@jfirebaugh
Copy link
Contributor Author

In debugging the query test failure, I think I found a flow soundness bug! facebook/flow#4580

These classes were a poor abstraction: they required highly conditional logic to satisfy the needs of all their consumers, and they were tightly coupled to each other. It's better to have each bucket class contain exactly the array/buffer properties it needs, and to keep corresponding arrays and buffers close to each other in code.

This brings the buffer classes much closer to their corresponding gl-native implementations.
Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

😍 Eliminating the `{Array,Buffer}Group abstractions makes the bucket code more explicit and much clearer.

Only one minor question: would it be worth extracting into a initializeBucket() helper the bucket initialization code that's repeated in all but the symbol bucket (and actually, there too, in SymbolBuffers)?

@jfirebaugh
Copy link
Contributor Author

There's definitely some commonality that deserves extraction, but I want to avoid creating an abstraction until it's clearer that we've got the right one. Some of the issues are:

  • SymbolBucket is an obvious outlier.
  • FillBucket is also an outlier -- it maintains two index buffers per vertex buffer.
  • I think we'll want to make the flow types involved here generic on the layout attributes, like they are in native.

@kkaefer had some good ideas in this vein in mapbox/mapbox-gl-native@2d36983; see my comments on mapbox/mapbox-gl-native#9468. (These changes were written out of that PR but I've pushed a branch to keep them around.)

@jfirebaugh jfirebaugh merged commit bb31e67 into master Aug 14, 2017
@jfirebaugh jfirebaugh deleted the bucket-interface branch August 14, 2017 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants