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

Map makes unnecessary repeated requests against a source's tile #4074

Open
daniel-j-h opened this issue May 4, 2024 · 16 comments
Open

Map makes unnecessary repeated requests against a source's tile #4074

daniel-j-h opened this issue May 4, 2024 · 16 comments
Labels
need more info Further information is requested

Comments

@daniel-j-h
Copy link

Hey folks, I ran into this issue when shipping an overview map displaying vector tiles with a single tile on zoom level zero.

Meaning

  • I have a vector tile source with a single tile encoding the whole world on zoom level 0; there are no other tiles
  • I restrict minzoom and maxzoom in maplibre's vector source, both set to 0
  • When the page loads the vector tile gets loaded once via the network (so far so good)
  • When I now zoom in and out, I can see unexpected additional requests against the very same vector tile

This is unexpected to see many more network requests against the very same tile at zoom level 0. I expected the tile to get loaded once and then the map over-zooming the encoded geometry as we zoom in and out without having to make the same request again and again.

It seems like the network requests are cached by the browser but not by the map. This is an issue in cases caching does not work e.g. as in protomaps/PMTiles#272 and ideally the map would not make network requests it doesn't need to make.


Here is a self-contained reproducible example with the demotiles for simplicity
https://gist.github.com/daniel-j-h/27f614132e4293bf5de1d3a613a225b7

Here I set minzoom and maxzoom to zero
https://gist.github.com/daniel-j-h/27f614132e4293bf5de1d3a613a225b7#file-map-html-L38-L39

Here are the corresponding docs

Note: The same behavior is visible when hard-coding a different zoom level than zero, I just thought this would be easiest to reproduce.

maplibre-gl-js version: 4.1.3 (latest)

browser: Firefox and Chrome on Ubuntu 20.04 LTS both affected

Here is a visual example of what I wanted to use this for: shipping an overview world map where zoom level 0 is perfectly fine and no other zoom levels are needed:

extract-layers

and here you can see the map making the very same network requests over and over again as we zoom in and out

network-requests

@HarelM
Copy link
Collaborator

HarelM commented May 4, 2024

When using the developer roll with cache disabled I'm not sure you get the desired effect.
Also I'm not sure what are the cache control of the tiles you are serving.
This is not to say that there isn't a bug here, but there is some code that takes care of caching inside maplibre.
Maybe the fact that you are using the same zoom level for min and max "confuses" the code.
Feel free to investigate this. 😃
The main code is in source cache and ajax classes.

@HarelM HarelM added the need more info Further information is requested label May 4, 2024
@daniel-j-h
Copy link
Author

I checked the dev tool disabled cache but that's not it. Here you can see the map still making multiple requests to the single vector tile. The request and response headers are identical by the way and caching by ETag and max-age tag looks fine, too.

Independent of browser caching what I don't understand is why over-zooming needs to make requests at all when minzoom and maxzoom is both set to zero. As in: there should only ever be a single network request to this vector tile.

cache1

@daniel-j-h
Copy link
Author

Another data point: if we set minzoom to 0 and maxzoom to 1 and zoom in we can see similar browser requests in the network tab that are not explainable. As in: if I zoom in all the way and out again, I get 30+ requests to the same tiles while zooming.

@HarelM
Copy link
Collaborator

HarelM commented May 4, 2024

Can you create a jsbin/stackblitz/codepen reproduction of this issue?
Sorry for nagging...

@daniel-j-h
Copy link
Author

Ah I thought the self-contained reproducible gist I shared above would work, but sure I can copy & paste it into a jsbin for you here you go: https://jsbin.com/bufiyejase/1/edit?html,output

Open the dev tools network tab and for example zoom all the way in, then zoom out again. Notice how maplibre makes repeated requests against the 0/0/0 tile while zooming. This is unexpected.

@HarelM
Copy link
Collaborator

HarelM commented May 5, 2024

0 might be a bit confusing due to world copies, so I used 4,4, which still has this issue.
I'm not sure I fully understand the code in here:

_updateRetainedTiles(idealTileIDs: Array<OverscaledTileID>, zoom: number): {[_: string]: OverscaledTileID} {

I also think this bug exists for a long time without anyone noticing.

Inside _addTile there's a call to the worker thread to get the relevant tile, which when zooming to zoom 5 seems to get the behavior wrong.
I need to see if this is just when setting the min and max zoom to be the same or a general issue in the code.

Feel free to investigate too as it might take me a while to find the time to dig into this.

CC: @msbarry as I know you are looking at this weird code...

@msbarry
Copy link
Contributor

msbarry commented May 6, 2024

I've noticed this behavior as well. I think the vector tile worker does actually need to re-process the tile since the layout properties need to be recomputed for each overzoom level, it shouldn't need to re-fetch. Since SourceCache only caches the entire fetched+parsed result it doesn't make any attempt to skip downloading the same data again.

This is on my radar to look into with the source cache improvements, it's very similar to avoiding duplicate fetches for raster-dem tiles used across terrain, hillshade, and countours. I think we want a layer between SourceCache (which is a bad name, it's actually staging the tile pyramid required to render the current scene) and the raw tiles we actually fetch that deduplicates and caches requests.

Some related issues:

  • currently source data loading/loaded events are identical for overzoomed tiles and fetched tiles, should we have separate events (or a way to differentiate source data events), for actually loading data from a source vs. when we finish parsing a fetched tile?
  • is fetching then parsing in the web worker necessary vs. fetching the tile in the main thread and transferring to the worker? It's easier to deduplicate requests if we do the latter

@HarelM
Copy link
Collaborator

HarelM commented May 6, 2024

Yes, passing the fetched data is a performance overhead.
Service worker might be a good fit for managing the cache stuff, I'm not sure if we can share it with regular workers, but it might be an interesting direction to pursue.

@msbarry
Copy link
Contributor

msbarry commented May 6, 2024

Yes, passing the fetched data is a performance overhead.

Can you elaborate? We pass the fetched data back from the worker to the main thread... If it's an array buffer then it should do a 0-copy transfer with the built-in transferrable utilities?

@HarelM
Copy link
Collaborator

HarelM commented May 6, 2024

You'll need to wait somehow on the main thread for the fetch to complete instead of sending the message to the worker and continue doing other stuff.
Might not be interesting, but worth thinking about.

@wagewarbler
Copy link
Contributor

Can confirm that I've also observed this same behavior. The behavior with source_cache is to preserve the most recent parent for cross-fading with new tiles as you zoom in for a more fluid transition between zoom levels. If you narrow the range of zoom levels down, I would expect this to "flatten" the tile cache pyramid so that your last parent is the max zoom, but I prob need to give the code another pass with an eye out for this specific behavior. This is something my team needs as well so will likely dig into it soon if someone doesn't get to it quicker.

@gvkhna
Copy link

gvkhna commented Dec 13, 2024

@HarelM You mentioned service workers, that's what's done here I found that's related.

https://thomasgauvin.com/writing/static-protomaps-on-cloudflare/

It seems this implementation comes with some initial performance penalties though.

@HarelM
Copy link
Collaborator

HarelM commented Dec 13, 2024

I meant browser's service worker, not a lambda like service worker which is equivalent to a backend from the browser perspective.

@gvkhna
Copy link

gvkhna commented Dec 13, 2024

@HarelM

The linked blog post uses browser service worker yes. I think this kind of solution may be the best bet.

In this article: https://thomasgauvin.com/writing/maps-web-app-with-protomaps-and-cloudflare/

Thomas transforms the range requests into tiled .mvt with a cloud worker and they become cacheable.

If this can be done client side with a browser service worker then they will be cacheable too.

@HarelM
Copy link
Collaborator

HarelM commented Dec 13, 2024

The browser's service worker main goal is to cache requests and allow some offline usage etc, it's a completely different thing in regards to cloudflare's service worker...

@gvkhna
Copy link

gvkhna commented Dec 13, 2024

@HarelM

Sorry if i'm not being clear, and this may be unrelated to this exact discussion. But my comment linked is what i'm referring to. The issue that pmtiles range requests don't seem to be cached by the browser, but .mvt files are.

I'm wondering if a browser service worker that transforms the pmtiles byte range request into a .mvt request or similar would allow it be disk cached... Or how we can get range requests cached by the browser.

protomaps/PMTiles#272 (comment)

NOTE: i'm broadly pontificating out loud about potential client-side solutions to cache pmtiles range requests. I myself am not clear if a browsers service worker would work for this use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need more info Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants