Bucket enhancements to allow property-only update

[anandthakker]

@lucaswoj I've been experimenting a bit with the "join new properties to existing geometries" problem. As you've suggested when we've discussed this, a couple of key enhancements to Bucket will be needed:

• Track a mapping from feature index to vertex array index range -- this would allow a source to know which parts of the paint-property-related vertex arrays to update for which features.
• Accept optional end indexes in populatePaintArrays -- currently, this method only takes a starting index and assumes that it should update from that index to the end of the current arrays, which makes it only suitable for populating the arrays the first time. Adding an end index parameter would make it straightforward to make updates to an already-populated array.

This PR includes draft versions of those changes, along with:

• A proof of concept updateProperties method, which I stuck onto WorkerTile just for demo/prototyping purposes (but which should in reality probably go into a custom source type that encapsulates the data-joining/updating)
• A pair of benchmarks -- dds_parse_bucket and dds_update_bucket -- to get a quick-and-dirty estimate of the perf gains from just updating versus a full parse. Results are pretty good at time of posting this = approx 43% gain:

[anandthakker]

With this method now accepting ending indexes, maybe it would be simpler require the end index params and move the responsibility for choosing group.layout.vertex.length - 1 out to the callers?

[lucaswoj]

That makes sense 👍

[mtirwin]

👀

[mourner]

Nice! What's up with the build failure — needs test updates as well?

[anandthakker]

@mourner Yep, tests needed small tweak. Just fixed in a556c97 along with the change to populatePaintArrays mentioned above

[anandthakker]

Ah, looks like some rendering tests are still broken. Will check those out soon.

[anandthakker]

@lucaswoj @mourner ^ the last few commits clean things up as follows:

• Accept an "array range" object ({startGroup, startVertex, endGroup, endVertex}) rather than individual indexes in populatePaintArrays.
• Rename _featureIndexToArrayIndex => featureIndexToArrayRange; updateBuffers => updatePaintArrays
• Contain _featureIndexToArrayRange wholly within bucket.js, by moving the responsibility for updating it into populatePaintArrays, because that seems like the most natural common code path for various bucket types to add paint data for a single feature.
• Fix tests, and add a simple test for updatePaintArrays

[anandthakker]

Removing "WIP" from the title now -- let me know if you think the changes in *bucket*.js look okay, and if so I will revert the temporary changes to worker_tile to make it mergeable

[mourner]

Should this comment be reformatted into JSDoc like rest of the code, while moving the note to a ticket or GH comment? It looks a bit like a TODO.

[anandthakker]

Ah @mourner sorry I wasn't clear in the comment--my intention was actually to revert this change entirely, excluding it from the PR and keeping the changes scoped to the Bucket classes. That's what I meant by "this might make more sense as part of a custom source type".

I guess a downside to that is that the dds_update_bucket benchmark would also have to go.

If yall think it's worth keeping this here, though, I'd be happy to clean it up (including the comment, as you said), and also to add some tests for it.

[anandthakker]

In preparation for a rebase, preserving some inline comments:

@mourner commented about fixing/removing the TODO-like comment above WorkerTile#updateProperties, and I responded:

sorry I wasn't clear in the comment--my intention was actually to revert this change entirely, excluding it from the PR and keeping the changes scoped to the Bucket classes. That's what I meant by "this might make more sense as part of a custom source type".

I guess a downside to that is that the dds_update_bucket benchmark would also have to go.

If yall think it's worth keeping this here, though, I'd be happy to clean it up (including the comment, as you said), and also to add some tests for it.

cc @lucaswoj for you to weigh in

[anandthakker]

Rebased (onto arrays-and-buffers branch, NOT master, so this PR is downstream of #2837 ). Previously branch was at 1f04efc

In other news, I guess rebasing doesn't mean losing the inline diff comments -- how nice!

[anandthakker]

I'm finding that there are some problems here relating to the arrays getting "transferred" from the worker back to the main thread after the initial load/parse:

Transferring apparently leaves the array "neutered" in the worker thread, so that it can't be updated. Moreover, when the bucket is reconstructed on the main thread, the transferred/serialized arrayGroups is currently not being unserialized and reinstated -- rather, it's only used to create bufferGroups.

[mourner]

Transferring apparently leaves the array "neutered" in the worker thread

Yeah, that's the point of transferring — moving data fast between threads without copy, but at the expense of not being able to access it after it was moved.

[anandthakker]

Yeah, makes sense. I'm thinking I'll play with either (a) transferring the
arraybuffers back to the worker for the property update task or (b) doing
the property update on the main thread.

I think either of these will require reconstructing the "arrayGroups"
object somehow -- for (a) I think I would have to replace the neutered
array buffers w the ones that I transferred back; for (b) I'd need to
create arrayGroups from the serialized arrays, similar to how
"arrayBuffers" is created, so that populatePaintArrays can be used.

On Sun, Jul 10, 2016 at 3:07 AM Vladimir Agafonkin [email protected]
wrote:

Transferring apparently leaves the array "neutered" in the worker thread

Yeah, that's the point of transferring — moving data fast between threads
without copy, but at the expense of not being able to access it after it
was moved.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#2812 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AEvmRwtpN1qCtqCEgXikJ_wQ6w-OPaO-ks5qUJosgaJpZM4JDP-w
.

[anandthakker]

I went with option (a), implemented in 5cfc637, and it works! Demo:

Those points are coming from a vector tile source; update() is hardcoded to send in random values for the dds property that's controlling circle-radius. (Code here -- relies on both this PR and #2667)

[anandthakker]

It occurs to me that this map should actually be this._featureIndexToArrayRange[interfaceName][featureIndex]

[anandthakker]

@lucaswoj Soon I will rebase to bring this up to date with the refactor in #2865; before I do, would love to get your 👍 / 👎 on each of the following changes that are currently included here:

1. Updates populatePaintArrays to (a) require both starting and ending indexes (in a combined arrayRange param), and (b) if a feature index is provided, update the bucket's feature-index-to-array-range mapping.
2. Add Bucket#updatePaintArrays which, given an array of feature properties, uses the feature-index-to-array-range map to correctly call populatePaintArrays for each of the new properties objects
3. Add Bucket#setArrayGroupData to allow a worker-side bucket to update its ArrayBuffer references after they've been transferred to the main thread (and thus neutered) and then transferred back.
4. Add WorkerTile#updateProperties, supported by items 1-3 above.

Number 4 raises a broader question: originally, I only included it so that yall could check out the benchmarks and such, assuming that in the end we would remove it and leave that logic as the responsibility of the dynamic-property-updating custom source implementation. But this does require that custom to reach pretty far into the GL JS's implementation details. Is this okay? Would it be better to perhaps include WorkerTile#updateProperties as part of the public api? Or even to include the dynamic-updating source as a core source type?

[lucaswoj]

1. Updates populatePaintArrays to (a) require both starting and ending indexes (in a combined arrayRange param), and (b) if a feature index is provided, update the bucket's feature-index-to-array-range mapping.

Sounds good 👍

2. Add Bucket#updatePaintArrays which, given an array of feature properties, uses the feature-index-to-array-range map to correctly call populatePaintArrays for each of the new properties objects

Sounds good 👍

3. Add Bucket#setArrayGroupData to allow a worker-side bucket to update its ArrayBuffer references after they've been transferred to the main thread (and thus neutered) and then transferred back.

/**
+ * Sets the underlying `arrayBuffer` instances for this bucket's array groups.
+ * When the arrayBuffers are transferred across threads with postMessage, the
+ * references to them on the sending thread become "neutered". This method
+ * allows for those arrayBuffers to be sent _back_ to the worker they came
+ * from.

This sounds potentially problematic. What happens if the main thread tries to render the bucket while its arrays are "missing"?

Is it possible to create a fresh paint array with the new data without transferring anything from the main thread back to the worker? I can't think of any reason why we'd need to read from the original paint arrays or why we have to use the same ArrayBuffer instance for the new paint array.

4. Add WorkerTile#updateProperties, supported by items 1-3 above.

Number 4 raises a broader question: originally, I only included it so that yall could check out the benchmarks and such, assuming that in the end we would remove it and leave that logic as the responsibility of the dynamic-property-updating custom source implementation. But this does require that custom to reach pretty far into the GL JS's implementation details. Is this okay? Would it be better to perhaps include WorkerTile#updateProperties as part of the public api? Or even to include the dynamic-updating source as a core source type?

I would prefer to refrain from making this a public API. You may add this code and use it for your vector tile property updater but we do not plan to provide support for or make any external promises about the stability of this API.

[anandthakker]

Thanks @lucaswoj !

This sounds potentially problematic. What happens if the main thread tries to render the bucket while its arrays are "missing"?

Doh! I hadn't thought of this.

Is it possible to create a fresh paint array with the new data without transferring anything from the main thread back to the worker?

Yeah, it does seem like this should be possible. Since we'd be talking about creating new paint arrays but not the corresponding "layout" arrays, doing this might involve some tweaks in a couple places, but pretty minor, I think.

I would prefer to refrain from making this a public API. You may add this code and use it for your vector tile property updater but we do not plan to provide support for or make any external promises about the stability of this API.

👍 this makes sense and sounds reasonable to me.

[anandthakker]

Rebased (previous: 5cfc637)

Is it possible to create a fresh paint array with the new data without transferring anything from the main thread back to the worker? I can't think of any reason why we'd need to read from the original paint arrays or why we have to use the same ArrayBuffer instance for the new paint array.

Also updated to reflect this suggestion, removing Bucket#setArrayGroupData in favor of Bucket#updatePaintBufferData, which, on the main-thread side, allows for updating with newly-created paint arrays.

Next steps:

• Add unit tests to cover new methods in Bucket, BufferGroup, and WorkerTile
• Fix benchmarks

[anandthakker]

Add unit tests to cover new methods in Bucket, BufferGroup, and WorkerTile

The tests now in place for Bucket#updatePaintVertexArrays, Bucket#updatePaintVertexBuffers, and WorkerTile#updateProperties should have this covered.

[anandthakker]

Rebasing to keep up with master. Previous head is 927c8d9

Branch

load-multiple-maps: 250 ms, loaded 6 maps.
buffer: 875 ms
dds-parse-bucket: 2,155 ms
dds-update-bucket: 1,203 ms
fps: 60 fps
frame-duration: 8.2 ms, 0% > 16ms
query-point: 0.77 ms
query-box: 52.55 ms
geojson-setdata-small: 10 ms
geojson-setdata-large: 102 ms

Master

load-multiple-maps: 864 ms, loaded 6 maps.
buffer: 874 ms
fps: 60 fps
frame-duration: 8.1 ms, 0% > 16ms
query-point: 0.76 ms
query-box: 51.31 ms
geojson-setdata-small: 11 ms
geojson-setdata-large: 104 ms

[AndyMoreland]

Hey folks,

I spoke with @ryanbaumann at my office a few days ago regarding some performance issues that I'm having with using mapbox-gl-js. He pointed me at this PR as a starting place for trying to improve hover / selection performance on maps with large numbers of geojson elements (40k is a reasonable target).

I've been doing some work based off of this PR, but it doesn't rebase cleanly against 0.24 anymore. I totally understand that this exact code probably won't be what gets merged into the mainline eventually, but is this a total fools errand? I'm primarily concerned about getting stuck permanently on my 0.22 fork if this gets abandoned.

Andy

[lucaswoj]

I cannot speak to @anandthakker's plans to maintain this branch.

We have plans to enable functionality like the functionality implemented in this pull request through custom source and layer types (#281 #2982 #2982). I suspect the final form of this feature will involve simplification of GL JS's architecture and emphasize object composition as the preferred way to create custom sources.

If you are looking for a way to improve the performance of hovering and selection, I recommend looking at #2874, other Map#setFilter performance improvements, and Map#queryRenderedFeatures performance improvements.

[anandthakker]

I cannot speak to @anandthakker's plans to maintain this branch.

Based on the most recent discussion in the PR here, I don't expect that this branch is going to make it into master. The work here was motivated by a project we're working on that requires joining changing property data with geometries provided by vector tiles. As discussed here, this is actually already possible via the 'custom source' interface, although that interface is likely to change as part of the architecture simplifications that @lucaswoj mentioned, and so isn't yet publicly documented.

@AndyMoreland I'll be open sourcing our approach to this, but it's going to take a few weeks due to constraints beyond my control.

[AndyMoreland]

Thanks for the update, both of you. It sounds like I should hold off for a bit on this until you get the new APIs standardized.

[ryanbaumann]

Linking to the categorical data-driven style method for performing property updates, which may be useful until vector tile property updates are merged into master.

https://www.mapbox.com/mapbox-gl-js/example/data-join/

[lucaswoj]

Closing this as "stale." We are working on a slightly different approach, allowing custom sources which adhere to a specific interface, here: https://github.com/mapbox/mapbox-gl-js/projects/2.