search

mapbox / mapbox-sdk-cs

C# libraries for Mapbox API layer
https://mapbox.github.io/mapbox-sdk-cs/
Other
20 stars 11 forks source link

Optimize http requests #40

Closed wilhelmberg closed 7 years ago

wilhelmberg commented 7 years ago

Bring multithreaded tile fetching back into core

wilhelmberg commented 7 years ago

@isiyu @MateoV @brnkhy @david-rhodes @parahunter

Could I get some 👀 from you guys.

This should bring in the first iteration of threaded tile fetching. Sorry @parahunter, it's event based 😏 I'm open for discussion and I think we haven't yet made a final decision which pattern to follow.

Tests work locally but fail on AppVeyor - looking into this right now.

wilhelmberg commented 7 years ago

Forgot to mention above: already merged master into this branch so merging should cause no troubles.

wilhelmberg commented 7 years ago

Ok, AppVeyor ✅ https://ci.appveyor.com/project/Mapbox/mapbox-sdk-unity-core/build/1.0.162

I introduced [Timeout()] for network related tests and the time span was too short for AppVeyor.

david-rhodes commented 7 years ago

@BergWerkGIS What should we specifically be looking for? I think a lot of stuff has changed here?

wilhelmberg commented 7 years ago

@david-rhodes I was thinking about a general review before merging.

david-rhodes commented 7 years ago

@BergWerkGIS Here are some of my notes:

Some of these are general notes for SDK, and not this PR, specifically. I'm fine merging this now just to keep things moving, however.

wilhelmberg commented 7 years ago

@david-rhodes thanks for taking a look.

  • It seems that threading is coupled specifically to Map.cs. Is there a way to abstract it further so that we can, for example, use threading in our SDK VectorTerrainSlippy example?

Of course we can/want to make further changes - this is just the first iteration of getting threading into our sdk.

My idea was to keep the user away from actual tile fetching as much as possible: Just set coordinates and zoom => get back notifications for each tile and when download queue is done. And the Map object seemed the right place - I suppose a user will always have some kind of (at least virtual) map object as a base for the scene.

I started reworking (far from finished) the old Slippy example to just need:

image

I guess that's obsolete now as the new Slippy has evolved quite a bit (MapController/MapVisualizers).

Going to look into integrating threading with MapController and MapVisualizer after some more memory profiling.

  • I'm wondering if this can't be contained as part of FileSource.cs or something. The threading and object pooling that @parahunter mentioned could both be handled here. This may make sense anyway for when we add request throttling/making requests for failed tiles, etc.

That's exactly what I thought to put into Map.cs. But yes, FileSource.cs might be a place to consider.

@object pooling: I think we have to balance speed and memory usage. With TileFetcher's MemoryCache I deliberately chose to just store raw byte[]s instead of .Net objects (Vector-/Raster-/ClassicRaster-Tile) to keep memory footprint as low possible while allowing for a lot of tiles to be cached. But we might change that to store the already initialized .Net objects.

  • You still have side effects in Map.cs. Let's ditch this.DownloadTiles(); in things like Zoom.

@david-rhodes Care to explain the side effects? Why ditch DownloadTiles()? I think changing zoom level is an important enough change of the properties of a map to trigger fetching of appropriate tiles, no?

If one wants more control over when tiles are fetched there are:

  • Dispose of UnityWebRequest.

Already had it in there, but I wasn't able to spot any differences so I removed it. Added again: https://github.com/mapbox/mapbox-sdk-unity-core/pull/40/commits/805ec81c41fd78d9ffea2d79cf4db24fc8dd9c9b#diff-87e63370b45a9b4cd7432bcdf87ff0e2R36

  • Add object pooling for requests and tiles (prevent unnecessary allocations and GC by reusing objects).

Sort of (partly) implemented via MemoryCache. Or are you thinking about something else./more elaborate?

I'm fine merging this now just to keep things moving, however.

🎉 🎉 🎉

david-rhodes commented 7 years ago

And the Map object seemed the right place - I suppose a user will always have some kind of (at least virtual) map object as a base for the scene.

This is true, but we can't assume everyone will use Map.cs, or MapController.cs, or any other version we make, for that matter. But, as you mentioned, we should try to keep requests hidden from developer, so their own implementation of a "map" can also make threaded requests without them having to know how threading works (I'm a threading newbie, for example). It seems like FileSource could be the gatekeeper.

Care to explain the side effects?

Say a developer wants to change MapId and Zoom or Center at the same time. This will result in multiple requests, right? With the first request being wrong?

I would prefer something like:

_map.Zoom = 15;
_map.MapId = "something";
_map.DownloadTiles();

But I guess this is the same as:

_map.DisableTileDownloading();
_map.Zoom = 15;
_map.MapId = "something";
_map.EnableTileDownloading();

I'll get back to this regarding object pooling.

wilhelmberg commented 7 years ago

Did some more memory profiling of the threading code (no Unity involved) and wasn't able to spot any major leaks.

Nevertheless I should spend some time on VectorTileCs #13 (Refactor processing of pbf bytes[]) asap.

That's the code I used for profiling (it's self contained - no references to other repos or nuget etc., latest DLLs of this branch already included):

--> mapbox-sdk-unity-core-profile-threading-master.zip

In case anyone wants access to the repo - ping me.