Don't mix coalesce calls from multiple GeoJSON sources (issue #5970) #5983
Conversation
Also: send back an "abandoned" callback for coalesced calls so that foreground doesn't leak callback handles. Add 'loadData' unit test for geojson_worker_source that simulates building up a queue of loadData messages.
|
Hrm... not too thrilled with the additional state tracking this solution requires. What would it take to be able to truly self-send the coalesce method? |
|
I did manual testing against the 2000 circle example from #5716 (comment) -- performance feels similar. |
I'm not sure -- I don't think we have a way to directly access the worker's message queue. Maybe we could build a wrapper/shadow of the message queue and push the self-send there? I don't see how we could manage the order correctly though, since we don't have a good way to know when we've temporarily emptied the real message queue (i.e. when it's time to insert our "self sent" messages). Self-sending would be a simplification, but it wouldn't address either of the issues in this PR -- we'd still have to maintain state separately for sources that share a worker, and we'd still have to send back "abandoned" callbacks to the foreground. |
src/source/geojson_worker_source.js
Outdated
| @@ -120,13 +124,19 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource { | |||
| * @param params.source The id of the source. | |||
| * @param callback | |||
| */ | |||
| loadData(params: LoadGeoJSONParameters, callback: Callback<void>) { | |||
| loadData(params: LoadGeoJSONParameters, callback: Callback<boolean>) { | |||
There was a problem hiding this comment.
Don't the params and callback need to be stored on a per-source basis as well?
I think the root problem here is that WorkerSources should be per-source, not per-source-type. The mapping from source ID to WorkerSource instance should be handled by Worker.
There was a problem hiding this comment.
Don't the params and callback need to be stored on a per-source basis as well?
Oh, of course. Good catch.
I think the root problem here is that WorkerSources should be per-source, not per-source-type. The mapping from source ID to WorkerSource instance should be handled by Worker.
🤔 That sounds convincing, although I'd like to handle it in a different PR. On a quick inspection, it looks like the StyleLayerIndex is the main thing that would remain shared between multiple WorkerSources?
- Store 'pendingCallback' and 'pendingLoadDataParams' per-source - Store all source-related parameters (with exception of super 'loaded' parameter) in one dictionary - Remove pending callbacks associated with a source when the source is removed - Handle case in which 'coalesce' arrives after source has been removed - Add unit test that would catch callbacks being shared between sources
|
I addressed the params/callback problem, and in the process noticed a further race condition between |
|
@ChrisLoer @jfirebaugh when a worker sends a message to the main thread, the response is queued right along with other main-thread-initiated messages, right? So another approach to the coalesce message would be to have the worker source send |
|
@anandthakker My first implementation worked with the worker sending an explicit coalesce message to the main thread -- I think it's pretty similar to the current approach of having the main thread |
|
@ChrisLoer this is subsumed by #5988 now, right? |
|
Yup. |
Fixes issue #5970.
Also: send back an "abandoned" callback for coalesced calls so that foreground doesn't leak callback handles.
Adds 'loadData' unit test for geojson_worker_source that simulates building up a queue of loadData messages.
/cc @ansis @jfirebaugh @anandthakker