mapbox / mapbox-sdk-cs

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

Consider StringBuilder to build request URLs #64

Open david-rhodes opened 7 years ago

david-rhodes commented 7 years ago

We have loads of string manipulation happening throughout the SDK to format request URLs. Because strings are immutable, this is creating lots of garbage for each request. I'm guessing each request uses 20+ temporary strings. Request 128 tiles and now you have problems.

Solution: use StringBuilder (and dependency injection?)

wilhelmberg commented 7 years ago

Or UriBuilder which seems to be appropriate for url manipulation. Maybe even in combination with HttpUtility.UrlEncode.

brnkhy commented 7 years ago

intantiating a new string builder won't be cheap either though. It'll also have to be collected just like string but probably even more costly. Maybe we can do some tricky stuff like having a static string builder in the IFileSource but such tricks are generally more prone/easier to introduce new bugs.

Skeet's article about string concetination; http://jonskeet.uk/csharp/stringbuilder.html I say we can try it and compare performance but I also think the performance different will be quite quite low and considering how much time it takes to monitor&compare performance results, we might not prioritize this.

david-rhodes commented 7 years ago

@brnkhy I read that just 3 separate concatenation statements justify the cost of the string builder object. That said, I was wondering how well a static string builder would work . . .

Go through and count how many concatenations happen to format just one URL. This is non-trivial if you want a performant slippy map.

brnkhy commented 7 years ago

@david-rhodes admittedly I'm not sure what to do about these stuff these days considering that they already announced .net 4.6 for unity. i.e. unity will be getting async programming with that, which might be the best replacement for tile.initialize we're talking in other ticket. Unity garbage collector will most likely change as well so all these micro optimizations, even though quite important for core, might a waste of time. Unity is targeting June for 2017 release by the way, not that far away either.