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

Bucket: Arrays and buffers #2837

Merged
merged 7 commits into from
Jul 14, 2016
Merged

Bucket: Arrays and buffers #2837

merged 7 commits into from
Jul 14, 2016

Conversation

jfirebaugh
Copy link
Contributor

Some refactoring of Bucket to reduce overhead, simplify object structure, improve naming conventions, and make the use of the terms "array" and "buffer" consistent (the latter always referring specifically to an instance of Buffer, not loosely as any kind of data collection).

cc @lucaswoj

@jfirebaugh jfirebaugh mentioned this pull request Jul 7, 2016
anandthakker pushed a commit that referenced this pull request Jul 7, 2016
@jfirebaugh jfirebaugh force-pushed the arrays-and-buffers branch 2 times, most recently from 2acb2e2 to e88a9f6 Compare July 7, 2016 23:11
* ?element2: ElementArrayType
* },
* paint: { [layerName]: PaintVertexArrayType, ... }
* vertexArray: VertexArrayType,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you choose to call this vertexArray instead of layoutArray?

Conceptually the paintArrays and layoutArray are both vertex arrays. The distinguishing characteristic is that one contains data from paint properties and the other data from layout properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I'll rename to layoutArray.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or how about layoutVertexArray, paintVertexArrays, layoutVertexBuffer, paintVertexBuffers?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's good too 👍

@lucaswoj
Copy link
Contributor

lucaswoj commented Jul 8, 2016

This is ready to :shipit: once you've addressed the actionable comments above ^


I have a bigger-picture refactoring of Bucket in mind that may be worth pursuing before porting this 😐 architecture to native.

The hierarchy of objects in our data system roughly looks like this:

Tile -> Bucket -> program interface -> array group -> layout and paint arrays

We could simplify by collapsing the hierarchy to:

Tile -> TileLayer

There would be a 1:1 correspondence between TileLayers and calls to drawElements. All information that was previously stored in the object hierarchy would be stored within TileLayer.

class TileLayer {
    programInterface: ...
    layoutArray: ...
    vertexArray: ...
    paintArrays: ...
    ...
}

We could also factor all TileLayer creation logic (i.e. addFeature) out into a new set of TileLayerFactory classes. This functionality is only used within the workers.

@jfirebaugh
Copy link
Contributor Author

Holding on merge here until post 0.21 release to avoid regression risk.

@lucaswoj
Copy link
Contributor

Ready to 🚢 ?

The prior implementation was a no-op, because it skipped a level in the object hierarchy.

Fixing this produced a meaningful benchmark improvement. Buffer benchmark before: 997ms; after: 895ms.
For a given bucket subclass, the VertexArrayType and optional ElementArrayType and ElementArrayType2 are constant for all instances. Initialize them once at startup.
* layout.vertex ⇢ vertexArray
* layout.element ⇢ elementArray
* layout.element2 ⇢ elementArray2
* paint ⇢ paintArrays
* layout.vertex ⇢ vertexBuffer
* layout.element ⇢ elementBuffer
* layout.element2 ⇢ elementBuffer2
* paint ⇢ paintBuffers
@jfirebaugh jfirebaugh force-pushed the arrays-and-buffers branch from 3cb4037 to 742b46d Compare July 14, 2016 00:17
@jfirebaugh jfirebaugh merged commit 742b46d into master Jul 14, 2016
@jfirebaugh jfirebaugh mentioned this pull request Jul 14, 2016
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.

2 participants