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

Allow limiting number of concurrent vector tile requests to prevent exhaustion of browser resources #13247

Open
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

jobblefrobble
Copy link

Launch Checklist

  • Make sure the PR title is descriptive and preferably reflects the change from the user's perspective.
  • Add additional detail and context in the PR description (with screenshots/videos if there are visual changes).
  • Manually test the debug page.
  • Write tests for all new functionality and make sure the CI checks pass.
  • Document any changes to public APIs. (Pending the actual config changes)
  • Post benchmark scores if the change could affect performance.
  • Tag @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes.
  • Tag @mapbox/gl-native if this PR includes shader changes or needs a native port.

Issue

This PR is to resolve an issue I raised around high numbers of vector tile sources on the map causing browser resources to be exhausted. Causing some tile requests to fail to be sent, and also any other concurrent requests from the browser to be stopped too.

Issue is here

Fix

  • The PR introduces a request queue for the loading of vector tiles, following the pattern used for fetching images (getImage in src/util/ajax.ts).

    • Requests will continue to be made, up until the number of concurrent requests becomes greater than our limit
    • After which subsequent requests are attached to the queue
    • When a request resolves, we advance the queue and make the next request
    • When all the callbacks for a particular request have been cancelled we remove the request from the queue
  • The solution is made slightly more complex than the image fetching by the inclusion of existing deduplication logic in the vector tile fetching.

    • A unit test has been added to make sure request deduping is still working as expected
    • Unit tests for the queueing behaviour have also been added

Help needed

Currently the queue limit is hardcoded to 50.
I had intended to set this as global config in a similar way to the image fetching mapboxgl.maxParallelImageRequests = 10 - however because the vector tile fetching mostly happens inside workers, it seemed like that the user-supplied config value was not being picked up.

If there's an established pattern for injecting config into the workers I would like to follow that! Otherwise any general guidance would be much appreciated 🙇

jobblefrobble and others added 30 commits July 11, 2024 17:05
…lback for a request that has actually been cancelled
… make array buffer generation an injected prop for testing
…quest-queue

Adding vector tile request queue
…-queued-vector-tiles

Add tests for queueing vector tiles
@CLAassistant
Copy link

CLAassistant commented Aug 7, 2024

CLA assistant check
All committers have signed the CLA.

@jobblefrobble jobblefrobble marked this pull request as ready for review November 26, 2024 16:53
@jobblefrobble jobblefrobble requested a review from a team as a code owner November 26, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants