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.

Sign up for GitHub

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

Jump to bottom

Manually strip urls for faster cache matching #8434

Merged
merged 1 commit into from
Jul 9, 2019
Merged

Manually strip urls for faster cache matching #8434

merged 1 commit into from
Jul 9, 2019

Conversation

mourner
Copy link
Member

@mourner mourner commented Jul 5, 2019

Closes #8431. A workaround for a known Chrome performance issue with match calls that use ignoreSearch: true.

@mourner mourner requested a review from asheemmamoowala July 5, 2019 12:49
@mourner mourner mentioned this pull request Jul 5, 2019
Closed
@mourner mourner requested a review from ansis July 8, 2019 16:52
asheemmamoowala
asheemmamoowala approved these changes Jul 8, 2019
View reviewed changes
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to measure this performance improvement? Does it show up in the benchmarks or the unit tests?

@mourner
Copy link
Member Author

mourner commented Jul 8, 2019

Is there a way to measure this performance improvement?

I did this by putting console.time/console.timeEnd in tile_request_cache.js cacheGet. You can see the problem by browsing the satellite.html debug page for a while — eventually cached tile loads start to take ~500ms each in Chrome. With this PR, it goes down to 20-50ms.

Does it show up in the benchmarks or the unit tests?

No, since we don't exercise tile loading in benchmarks and don't have unit tests run in Chrome.

@asheemmamoowala
Copy link
Contributor

I did this by putting console.time/console.timeEnd in tile_request_cache.js cacheGet. You can see the problem by browsing the satellite.html debug page for a while

I tried this on Chrome and see the opposite results from what I would expect:

Change Avg. time (ms)
With no changes 127.858224
With strippedURL + ignoringSearch 157.0550753
With this PR 189.2290455

eventually cached tile loads start to take ~500ms each in Chrome. With this PR, it goes down to 20-50ms.

The above numbers are for ~1000-1500 tiles in the cache. What range of cached tile size should the performance improvement apply to?

@mourner
Copy link
Member Author

mourner commented Jul 8, 2019

@asheemmamoowala are you sure you're timing correctly? Note that you have to time by request.url, otherwise you might capture the wrong time invervals (relating to different tiles). I just opened satellite.html in master, adding console.time('get' + request.url) + timeEnd in the relevant place, and I'm seeing ~2500ms per tile. This is with just 525 entries. After switching to this branch, it gets down to 200-300ms.

@mourner
Copy link
Member Author

mourner commented Jul 8, 2019

The above numbers are for ~1000-1500 tiles in the cache. What range of cached tile size should the performance improvement apply to?

The bigger the number, the bigger the perf difference, as you can read in the linked Chrome issue — time increases linearly with the amount of stored objects. We have a manual limit of 500 tiles for the cache, but the regression is still severe.

@ansis
Copy link
Contributor

ansis commented Jul 9, 2019

I did this by putting console.time/console.timeEnd in tile_request_cache.js cacheGet. You can see the problem by browsing the satellite.html debug page for a while — eventually cached tile loads start to take ~500ms each in Chrome. With this PR, it goes down to 20-50ms.

I'm seeing similar numbers! Looks great.

@asheemmamoowala
Copy link
Contributor

Was able to reproduce the results with pitched satellite maps.

Change Avg. time (ms)
With no changes 20000.858224
With strippedURL + ignoringSearch 3000.0550753
With this PR 500.2290455

All of these are with a cache that is full. So the results are worst case scenario for frequent map users.

@mourner mourner merged commit 20c9090 into master Jul 9, 2019
@mourner mourner deleted the fix-cache-match-perf branch July 9, 2019 17:17
mourner added a commit that referenced this pull request Jul 9, 2019
@mourner
manually strip urls for faster cache matching (#8434)
0089f1f
@mourner mourner mentioned this pull request Jul 9, 2019
Merged
mourner added a commit that referenced this pull request Jul 9, 2019
@mourner
manually strip urls for faster cache matching (#8434) (#8449)
9235d6a
katydecorah pushed a commit that referenced this pull request Jul 18, 2019
Merge branch 'publisher-production' into feedback
380377f 
* publisher-production:
  [docs] fix SDK support for symbol-sort-key (#8501)
  v1.1.1
  fix #8481, unbounded memory growth in IE (#8490)
  fix race condition between caching and aborting (#8472)
  swap map-tiles example to use https (#8482)
  set antialias to true for custom layers and extrusion examples (#8474)
  update examples to use mapbox-gl-geocoder v4.4.1 (#8451)
  manually strip urls for faster cache matching (#8434) (#8449)
peterqliu added a commit that referenced this pull request Jul 19, 2019
@peterqliu
merge publisher-production into master (#8519)
50c1ca6 
* [docs] adds docs-wide search component (#8349)

* update dr-ui, mr-ui

* add search component

* adjust columns to prevent overlap

* v1.1.0-beta.1 (#8353)

* Update CHANGELOG.md for v1.1.0-beta.1 release
* Bump package.json version to 1.1.0-beta.1

* [docs] updates to search component and layout (#8362)

* do not index /api sidebar

* exclude code snippets in examples from being indexed

* Update docs-page-shell

* update dr-ui, react, react-dom

* adjust columns to fit search and navigation better

* [docs] Replace example with a third-party raster source (#8274)

* remove Add a raster tile source example

* revert "remove Add a raster tile source example"

This reverts commit ea8bf28.

* add third-party tiles to map-tiles example

* fix attribution

* rm unused var

* Add mapbox-gl-framerate to plugins list (#8356)

* remove old plugins page and update reference to the maintained version (#8369)

* Add link to iOS SDK documentation (#8359)

* add ios sdk link per comment from stephen g

* add new link from minh and reword

* Include Hiragana and Katakana code blocks for local glyph generation (#8365) (#8378)

* document the tile size drawn with showTileBoundaries (#8379)

* use browser `Cache` interface to cache tiles (#8363)

Recent pricing changes introduced a `sku=` query parameter that changes with every map load. This defeats the browser's ability to cache these tiles. We're working around that by implementing our own caching with the new `Cache` api.

- skips caching tiles that expire soon
- only caches mapbox tiles (no 3rd party tiles, styles, etc because the browser should cache these fine)
- does not work in IE, Safari

Browser tests can be found in debug/cache_api.html

mapboxgl.clearStorage added to provide a way to clear the cache.

* fix cache support check for enforcing size limit

This was throwing errors in browsers that don't support the cache api
but wasn't previously noticed because it was not breaking anything.

* Fix background rotation hovering (#8367)

* update changelog

* [email protected] (#8297)

* [email protected]

* add one more file to  v4.4.0

* fix Response construction in Edge, fix #8391 (#8392) (#8394)

Edge does not support constructing responses with readable streams.

* v1.1.0-beta.2 (#8395)

* Add sdk support spec section for text-radial-offset (#8384) (#8401)

This PR also assigns SDK versions for the `text-variable-anchor`
property (both `text-variable-anchor` and `text-radial-offset`
are the parts of the variable text placement feature).

* fix #8411, fetch in case of cache error (#8415) (#8417)

* fix #8411, fetch in case of cache error

* Use warnOnce

* v1.1.0 & [email protected] (#8408)

* Update style-spec version & changelog for [email protected]

- Update style-spec/CHANGELOG.md to add bugfix items
- Bump style-spec/package.json version to 13.7.1 (patch)

* Update mapbox-gl package version to 1.1.0 for final release

* Add missing style-spec changelog entry

* Fix yarn.lock not installing under CI by auto-updating dependencies

* manually strip urls for faster cache matching (#8434) (#8449)

* update examples to use mapbox-gl-geocoder v4.4.1 (#8451)

* set antialias to true for custom layers and extrusion examples (#8474)

* swap map-tiles example to use https (#8482)

* fix race condition between caching and aborting (#8472)

fix #8470, continuous memory usage growth

* fix #8481, unbounded memory growth in IE (#8490)

Cancellable requests to workers should never call back after they have
been cancelled.

* v1.1.1

* [docs] fix SDK support for symbol-sort-key (#8501)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asheemmamoowala asheemmamoowala asheemmamoowala approved these changes

@ansis ansis ansis approved these changes

Assignees
No one assigned
Labels
release blocker ⛔
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

1.1.0 release tile load tiems performs poorly compared to 0.54
3 participants
@mourner @asheemmamoowala @ansis