MGLComputedShapeSource/CustomGeometrySource.java shouldn't create thread pools

[ChrisLoer]

The core CustomGeometrySource is configured with a "fetch tile function" that glues out into the platform-specific wrappers (and eventually the user-provided custom implementation). The darwin implementation of this function is in MGLComputedShapeSource.mm and the Android implementation is in CustomGeometrySource.java. Both implementations follow a similar pattern:

• Wrap the tile ID up in a "request" object
• Pass that request object off to be run on a shared pool of 4 worker threads
• When processing a request on the worker thread, synchronously call into the user-provided custom source to get data (id<MGLComputedShapeSourceDataSource> on darwin, GeometryTileProvider on Android), and then once the synchronous call returns, queue an asynchronous setTileData callback that feeds back into the core CustomGeometrySource

I think this model would make sense if the primary job of the user-provided tile provider was to run some computationally expensive operation, but it doesn't seem to me like a very good fit for the case where the tile provider's job involves asynchronously requesting resources itself. Because the provider doesn't have access to a callback, it needs to block on asynchronous requests, which is an awkward interface and also opens the potential for clogging up the 4 worker threads with blocking requests. Also, this forces the provider to support synchronized cross-thread access to itself (a bit of an anti-pattern), even when it's not necessary.

I think it would be better if we leave concurrency choices to the user, so for instance GeometryTileProvider::getFeaturesForBounds would take a callback as an argument (some kind of wrapper on the core ActorRef that's being used for the underlying callback). Then, for someone implementing GeometryTileProvider:

• If the computation is simple, just do it synchronously and call the callback immediately.
• If the computation depends on an asynchronous resource like a network or database request, go ahead and make the request and pass the callback through. Don't worry about threads, that's an implementation detail of your network/database layer.
• If you actually have a lot of CPU work to do and you want to fire up worker threads and give them a queue of tasks, fine go ahead and do it, and it's on you to think about how to manage concurrency.

/cc @asheemmamoowala @jfirebaugh @1ec5 @ivovandongen

[mb12]

@ChrisLoer Instead of exposing CustomGeometrySource from the SDK, an alternative would be to implement it as a plugin. The plugin would run a simple http server and use run time styling api to add a new source with http://localhost:<port_number> tile template urls.

[ChrisLoer]

Capturing from conversation with @LukasPaczos regarding the changes in #12509:

Regardless of the threading model we use for CustomGeometrySource, we want to gracefully handle the case where the source is being frequently invalidated/updated, in which case it will be common to have new requests for a tile/region come in before old requests have completed.

• If the custom provider has two live callbacks A and B for the same region/tile, it may call them out of order
• If we null out pending callbacks when a tile gets re-requested, we'll have correctness, but also a potential starvation situation when the updates come in faster than individual requests can finish.

I think ideally we'd implement a coalescing strategy in CustomTileLoader::invalidateRegion (or somewhere around there) so that in-progress requests would finish, and the customer-provided getFeaturesForBounds wouldn't have to worry about ordering and could count on its results being used (if only briefly).

[1ec5]

The original implementation for iOS and mbgl did provide for asynchronous tile production, but in #6940 (comment) I asked @JesseCrocker to consider a synchronous API instead, since delegate or data source methods that rely on a passed-in callback to be called tend to be error-prone. It’s easy to forget to call the callback, though the fact that the callback is passed in should mitigate that danger somewhat. I had been thinking primarily of MapKit’s -[MKTileOverlay URLForTilePath:], an API that many developers specifically requested before the custom source feature landed. However, I didn’t pay enough attention to its asynchronous counterpart, -[MKTileOverlay loadTileAtPath:result:]. (“Path” in this case refers to MKTileOverlayPath, a struct we should’ve copied to minimize parameter count.)

Can the new method be implemented alongside the existing ones? I think it would be fine if the developer has to choose between -[MGLComputedShapeSourceDataSource featuresInTileAtX:y:zoomLevel:] and -[MGLComputedShapeSourceDataSource loadFeaturesInTileAtX:y:zoomLevel:result:].

[stale]

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

[stale]

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.