-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
per-SourceType WorkerSource -> per-source-instance WorkerSource #5988
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 making each worker source simpler is great, and I'm not really worried about consequences for custom sources
src/source/geojson_source.js
Outdated
@@ -239,6 +239,7 @@ class GeoJSONSource extends Evented implements Source { | |||
} | |||
|
|||
onRemove() { | |||
// FIX: Why do we broadcast instead of sending to this.workerID? | |||
this.dispatcher.broadcast('removeSource', { type: this.type, source: this.id }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point -- a VectorTileSource has multiple corresponding workers, but a GeoJSONSource doesn't.
58f3642
to
6109cd2
Compare
Since we reverted the GeoJSON coalescing changes, I cherry picked them into this branch and re-based it on top of
|
@mollymerp, I made analogous changes to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisLoer I think this looks great, and 👍 on the narrow fix for the removeSource
issue on GeoJSONSource.
src/source/geojson_worker_source.js
Outdated
_state: SourceState; | ||
_pendingCallback: Callback<boolean>; | ||
_pendingLoadDataParams: LoadGeoJSONParameters; | ||
_geoJSONIndex: GeoJSONIndex // object mapping source ids to geojson-vt-like tile indexes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the line comment is now obsolete
@@ -86,10 +86,10 @@ class Actor { | |||
// data.type == 'loadTile', 'removeTile', etc. | |||
this.parent[data.type](data.sourceMapId, deserialize(data.data), done); | |||
} else if (typeof data.id !== 'undefined' && this.parent.getWorkerSource) { | |||
// data.type == sourcetype.method | |||
// data.type == sourcetype.sourcename.method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: also update the doc comment above send()
to reflect this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, changes on RasterDEMWorkerSource
look good to me!
f97c6fa
to
4ea6197
Compare
OK, updated comments and squashed to two commits: (1) re-introduces GeoJSON loadData coalescing, and (2) does the WorkerSource re-factoring. My plan is still to hold off merging this until after we've released .44, just in case... |
4ea6197
to
ae67df9
Compare
@ChrisLoer sure thing--hopefully the unit tests capture a bunch of this, but for integration testing/sanity checking here's what I used:
to adapt it you will need to:
|
Thanks @sbma44! Looks like it's coming through fine. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good to me, but it might be worth checking the benchmarks?
function createWorker() { | ||
const worker = new GeoJSONWorkerSource(null, layerIndex); | ||
|
||
// Making the call to loadGeoJSON to asynchronous |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - tiny typo
Making the call to loadGeoJSON
toasynchronous
- Instead of making WorkerSource implementations responsible for tracking which source's messages they're processing, instantiate a different WorkerSource for each source, and make Worker responsible for forwarding messages to the correct instance. - Simplifies GeoJSONWorkerSource's coalescing logic. - Introduces a somewhat subtle requirement for Source implementers: any Source that sends a 'removeSource' message is responsible for sending no further messages. To re-add a Source with the same ID, create a new Source object and start sending messages. A new WorkerSource will be instantiated with matching state.
ae67df9
to
ada68c9
Compare
Benchmark results don't seem to show any big changes: https://bl.ocks.org/anonymous/raw/9187a9a85d29a307fe03fe1f86f3d61c/ |
Based on @jfirebaugh 's feedback on #5983, I started exploring instantiating
WorkerSource
s on a per-source basis instead of a per-source-type basis.The initial set of changes were not too bad, and it allows for a nice simplification of geojson_worker_source that still supports the
setData
coalescing behavior.My biggest concern is how this will affect custom sources -- requiring a source name to be included (either in parameters or in the "data.type" argument) is probably breaking the implied API?
removeSource
also needs some re-thinking to handle messages that arrive after a source has been removed (or, moving even more into edge-case territory, to handle messages that were sent to a previous instantiation of a source, which has been removed and then re-added).The unit tests that handle multiple sources are currently broken, but render/query tests pass.
/cc @jfirebaugh @anandthakker