Skip to content
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

"canonicalize" tile urls from TileJSON so they use config.API_URL #7400

Closed
mollymerp opened this issue Oct 11, 2018 · 6 comments · Fixed by #7594
Closed

"canonicalize" tile urls from TileJSON so they use config.API_URL #7400

mollymerp opened this issue Oct 11, 2018 · 6 comments · Fixed by #7594
Assignees

Comments

@mollymerp
Copy link
Contributor

gl-native has a step where it converts mapbox.com tile urls to use the mapbox:// prefix so that they ultimately fire requests at the baseApiUrl domain. we should add behavior in mapbox-gl-js as well, I think.

// Return a "mapbox://tiles/..." URL (suitable for normalizeTileURL) for the given Mapbox tile URL.
std::string canonicalizeTileURL(const std::string& url, style::SourceType, uint16_t tileSize);

https://github.com/mapbox/mapbox-gl-native/blob/f3341dd589c6330c0cfd6e8e381398d08493b48a/src/mbgl/util/mapbox.hpp#L21-L22

we should also consider adding a setter/getter for API_URL (native exposes these functions here)

@kkaefer
Copy link
Contributor

kkaefer commented Oct 15, 2018

Hm, the main reason this function exists in Native is for caching: the TileJSON endpoints from api.mapbox.com serve tile template URLs (in the tiles key) that are resolved to real HTTP URLs. For caching, however, we're canonicalizing them to mapbox:// so that we don't cache a.tiles.mapbox.com and b.tiles.mapbox.com twice even though they serve the same resource. They also contain access token which we're stripping for cache lookups so that we don't have a per-access token cache.

@kkaefer
Copy link
Contributor

kkaefer commented Oct 15, 2018

sorry, rereading the title of this issue and it's exactly what we use it for 😅

@mollymerp
Copy link
Contributor Author

@kkaefer good to know about the caching behavior. fwiw I took a closer look at the native codebase, and confirmed that native isn't making requests to the {a/b}.tiles.mapbox.com endpoints – tile URLs are normalized to "https://api.mapbox.com/v4/tileset.id/z/x/y.vector.pbf" using the baseURL. Making this change in JS would change existing behavior, but I'm not sure it would count as a breaking change.

@kkaefer
Copy link
Contributor

kkaefer commented Oct 16, 2018

Yeah good point. Mapbox TileJSONs return a tiles array with
[ "http://a.tiles.mapbox.com/v4/...", "http://b.tiles.mapbox.com/v4/..." ]. On Native, we resolve that back to a mapbox:// URL for the reasons stated above. The reason we have two different tile URLs here is to increase the number of simultaneous connections: Many browsers (used to?) limit the number of simultaneous connections per hostname to something between 2 and 6. We should revisit this now that we are using the Fetch API.

Apart from the number of simultaneous connections, another consideration is also DNS resolution: We're typically loading a style from api.mapbox.com, then tiles from *.tiles.mapbox.com. We're also doing a request to api.tiles.mapbox.com. This means that we're doing at least 4 DNS queries when loading a map. Reducing those to 1-2 might be worth the concurrency penalty if it still exists.

@mollymerp
Copy link
Contributor Author

mollymerp commented Oct 19, 2018

according to the internet the minimum limit for browsers we support is 6 🤔

image

I can run some benchmarks on various browsers to see if this change would affect network request performance. Are there any other metrics we'd want to collect before making this change?

@kkaefer
Copy link
Contributor

kkaefer commented Oct 19, 2018

hm, limiting to 6 concurrent connections may actually be beneficial in many situations since it avoids overloading the network. I think it's worth trying and benchmarking how this changes load times. The "Additional Tools for Xcode include a Network Link Conditioner prefpane that allows controlling the bandwidth and other network properties. Note that as of macOS Mojave you'll have to manually copy it to /Library/PreferencePanes, then restart your Settings.app

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants