Avoid XHR when possible for faster raster tile loading #7008

mourner · 2018-07-23T09:48:40Z

A proposal for addressing #6643. If we see that a raster tile we requested with XHR should be cached for a long time (currently set to 1 hr), we switch from XHR to simple new Image tile loads for that source. This fixes a lot of the jank when using raster tile sources.

Launch Checklist

briefly describe the changes in this PR
~~write tests for all new functionality~~ (we don't have tests for image loading, and they probably won't be useful anyway as long as we use jsdom and stubbing)
~~document any changes to public APIs~~
post benchmark scores
manually test the debug page

hyperknot · 2018-07-23T11:23:21Z

src/util/ajax.js

+        }
+        img.onload = () => callback(null, img);
+        img.src = url;
+        return {cancel: () => { img.src = transparentPngUrl; }};


Shouldn't callback(err) be handled here as well?

I think an onerror handler would solve it.

Good point, added.

mourner · 2018-07-23T12:16:11Z

Ran LayerRaster benchmarks — they are unaffected because they don't exercise the performance impact of tile loading.

hyperknot · 2018-07-23T12:52:15Z

XHR heuristics works perfectly, both cases are working well.

jfirebaugh · 2018-07-23T20:13:35Z

What are you using to evaluate this versus the prior behavior? In light testing, I'm not sure I can distinguish the difference.

mourner · 2018-07-23T20:17:13Z

@jfirebaugh I'm using the original test case here https://codesandbox.io/s/zl761nnr3m — the difference is very noticeable, especially if you have #6995 (another big perf hit) pulled in when comparing.

hyperknot · 2018-07-23T20:17:40Z

Here is a source with short expires time:

https://a.tile.opentopomap.org/{z}/{x}/{y}.png

And Mapbox satellite for example is with long expires time.

jfirebaugh · 2018-07-23T20:23:21Z

I'm comparing e9e08e2 and f3f9357 side-by-side on each half of an external monitor and I can't really tell the difference. You're saying it should be really obvious?

hyperknot · 2018-07-23T20:29:53Z

@jfirebaugh make sure you don't have developer tools open, as that introduced janks for me in Chrome.

old: https://codesandbox.io/s/zl761nnr3m
new (both PRs merged): https://codepen.io/hyperknot/full/vaxvJN/

jfirebaugh · 2018-07-23T20:37:25Z

This is with developer tools closed. Subjectively, the behavior I see is:

Approximately equal jank between the two. With this PR, maybe slightly less, but that also might be conditioning.
This PR shows less blank white space when zooming out.
This PR shows more pixelated, overzoomed tiles when zooming in.

mourner · 2018-07-23T20:46:27Z

@jfirebaugh on the examples above (make sure both are viewed fullscreen), I get heavy stuttering on the old example (on my MBP Retina 15'' screen), especially in the middle of zooming in and zooming out, and almost no jank on the new one.

Perf recording before (optimize-tile-retention) (notice the red dots and big gaps):

After (optimize-tile-retention + faster-raster-tiles-load merged in):

I also noticed like @hyperknot that it still stutters when devtools are open, but stops stuttering when you start recording a timeline perf profile — I guess Chrome disables some debug stuff while recording.

jfirebaugh · 2018-07-23T20:49:56Z

With developer tools open, looking at a performance recording, it looks like the time spent sending the requests might just be delayed until the end of the frame, rather than interspersed with JS execution:

mourner · 2018-07-23T20:56:32Z

Here's another comparison, JS profiler. Before:

After:

Also, subjectively, if I disable the fly animation and just browse the map fullscreen, it's barely usable before the PR with unbearable stuttering, and smooth and pleasant after.

jfirebaugh · 2018-07-23T20:56:33Z

Yeah, I don't see results as definitive as that. I'm testing locally with this file: https://gist.github.com/jfirebaugh/7cf3e57c359513f980d6aad56283d75e.

I wonder what causes such different results for us?

mourner · 2018-07-23T21:02:20Z

@jfirebaugh I think it's important to have all the other raster perf PRs merged in for this to have pronounced effect. I just rebased both PRs (this and #6995) on master. Now can you compare this PR vs this PR + #6995 merged?

jfirebaugh · 2018-07-23T21:14:50Z

I've been testing with optimize-tile-retention rebased on master and faster-raster-tiles-load rebased on top of that.

Does someone else want to step in and try? Must be some other variable unaccounted for here.

hyperknot · 2018-07-23T21:22:31Z

I've ported the example to OpenLayers, with a similar animation (the easing is different).
In my opinion OL is super smooth even with developer tools open. FPS meter shows constant 60 FPS with one or two occasional frames at 30 FPS, or maybe 20 FPS.

https://611v4nk57z.codesandbox.io/
https://codesandbox.io/s/611v4nk57z

Addresses #6643.

mourner · 2018-08-02T15:21:30Z

@hyperknot @jfirebaugh I just pushed a new WIP branch based on this one that adds a basic image queue (cbb059f), limiting parallel image loads to 16 at a time. Can you try it out and see if it makes a difference for performance?

mourner · 2018-09-24T09:52:05Z

@hyperknot any comment on the above?

hyperknot · 2018-09-24T09:54:54Z

@mourner sorry for not responding earlier. So do I understand right that the now stable release (0.49) has the previously discussed improvements, but not the tile queue one? And for tile queue I should check out cbb059f?

mourner · 2018-09-24T10:10:05Z

@hyperknot the last release includes all improvements except this PR (avoiding XHR for image loading). And additionally cbb059f explores the concept of limiting concurrency for image loads — let's see if it's any faster than this branch.

hyperknot · 2018-09-24T10:11:56Z

@mourner OK, I'll check out the improvements and report back today.

mourner · 2018-10-17T20:06:08Z

@hyperknot any updates?

hyperknot · 2018-10-17T21:09:34Z

Oh, sorry, I promised to get back but I didn't. I'll do it now.

hyperknot · 2018-10-18T16:02:31Z

@mourner this is absolutely wonderful performance wise! The 2 fps parts are now totally gone, even with console open.

I did a comparison between 0.50 and cbb059f here:
https://codepen.io/hyperknot/pen/bmvzQx
https://codepen.io/hyperknot/pen/xyWMMO

My only problem now is that I'm yet 100% confident in how the code is implemented. What isn't clear is both handleImageLoad and getImage checks for numImageLoads < > maxParallelImageLoads.
Shouldn't one of them be the reference for this test?

Also, are you sure it can never happen that the queue explodes? I mean getImage adds handlers for handleImageLoad which calls getImage.

Having said that, if all the delicate parts of these are thought out by you then I'm happy with the added huge performance improvements!

mourner · 2018-10-18T16:13:30Z

@hyperknot awesome, thanks a lot for checking! This was a quick proof of concept so likely pretty buggy — and now since this seems promising, I'll probably explore it more. Note that while FPS is improved, the branch codepen version seems to have more blurry tiles when zooming in because new ones aren't loaded fast enough.

We're also exploring raster loading performance from another angle in #7405, which will hopefully bring more improvements too.

hyperknot · 2018-10-18T17:00:36Z

But blurry tiles are good, aren't they? I mean blurry means that smooth animation is more important than title loading, which is exactly what should happen. Maybe it can be polished so that the tiles at the center of the screen take higher priority then the ones at the edges though.

…

On 2018. Oct 18., Thu at 18:13, Vladimir Agafonkin ***@***.***> wrote: @hyperknot <https://github.com/hyperknot> awesome, thanks a lot for checking! This was a quick proof of concept so likely pretty buggy — and now since this seems promising, I'll probably explore it more. Note that while FPS is improved, the branch codepen version seems to have more blurry tiles when zooming in because new ones aren't loaded fast enough. We're also exploring raster loading performance from another angle in #7405 <#7405>, which will hopefully bring more improvements too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7008 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAeKj4LsKGQa0-vNd1V-gytd1zjV47k8ks5umKjFgaJpZM4VasEq> .

hyperknot · 2018-10-18T23:21:54Z

OK, I had some proper look again, at how is blurriness handled, and found some interesting discoveries.

The old behaviour, in the Mapbox GL 0.50 example goes terrible if I select Fast 3G network condition + disable caching in Chrome. Like many second long pauses on totally frozen screen.
The tilequeue version is insanely better in this condition. Like it's actually usable. Blurriness is exactly how it should be.
The 0.50 version really doesn't handle packet loss well. With a 10% or 20% packet loss in Network Link Conditioner, it really quickly generates 3000+ requests, most of them cancelled. Tilequeue handles this case much better as well.

I was using this custom profile in NLC (and "Online" in the browser), it was really good to test this behaviour:

As the difference in behaviour is extremely big, I even recommend making a universal tile queue and use it for vector tiles as well maybe? This is something which we never see when developing on reliable network connections but mobile clients in the real world can have really high packet losses.

kkaefer · 2018-10-19T11:44:59Z

I can confirm that cbb059f is much smoother on my computer as well. It seems to load the maximum number of image loads that are loaded in parallel. However, I'm still seeing more than one image per frame being uploaded (call to texImage2D). That's something that we should limit to 1 image per second as well, i.e. only fire one onload event per animation frame.

kkaefer · 2018-10-19T11:45:34Z

@hyperknot thank you for these wonderful test cases; they're very useful for debugging this issue!

mourner · 2018-11-27T23:40:58Z

I have a feeling this PR won’t have much effect after #7497, so I’m going to close for now.

mourner requested a review from jfirebaugh July 23, 2018 09:48

mourner mentioned this pull request Jul 23, 2018

Raster tile loading blocking UI, FPS < 2.0 #6643

Closed

hyperknot reviewed Jul 23, 2018

View reviewed changes

mourner force-pushed the faster-raster-tiles-load branch from 3e19947 to f3f9357 Compare July 23, 2018 12:16

jfirebaugh closed this Jul 23, 2018

jfirebaugh reopened this Jul 23, 2018

mourner force-pushed the faster-raster-tiles-load branch from f3f9357 to bbbea02 Compare July 23, 2018 20:58

mourner mentioned this pull request Jul 27, 2018

Don't load tiles that will be shown in a very small number of frames while animating #5482

Open

mourner force-pushed the faster-raster-tiles-load branch 2 times, most recently from 98aa79b to 471c785 Compare July 31, 2018 11:17

mourner added 4 commits August 2, 2018 17:50

use non-XHR raster tile loads when possible

435159c

Addresses #6643.

dry up dem source, inherit xhr-avoiding logic

3d344dc

add error handling for non-xhr image loads

4c4e47d

avoid calling onload for cancelled raster tiles

8a0da8f

mourner force-pushed the faster-raster-tiles-load branch from 471c785 to 8a0da8f Compare August 2, 2018 14:51

hyperknot mentioned this pull request Oct 18, 2018

Reduce slowness when loading many raster tiles at once #7405

Open

5 tasks

mourner mentioned this pull request Oct 22, 2018

Limit the number of concurrent image loads #7460

Closed

mourner closed this Nov 27, 2018

asheemmamoowala deleted the faster-raster-tiles-load branch March 9, 2021 05:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid XHR when possible for faster raster tile loading #7008

Avoid XHR when possible for faster raster tile loading #7008

mourner commented Jul 23, 2018 •

edited

Loading

hyperknot Jul 23, 2018

hyperknot Jul 23, 2018 •

edited

Loading

mourner Jul 23, 2018

mourner commented Jul 23, 2018

hyperknot commented Jul 23, 2018

jfirebaugh commented Jul 23, 2018

mourner commented Jul 23, 2018

hyperknot commented Jul 23, 2018 •

edited

Loading

jfirebaugh commented Jul 23, 2018

hyperknot commented Jul 23, 2018

jfirebaugh commented Jul 23, 2018

mourner commented Jul 23, 2018

jfirebaugh commented Jul 23, 2018

mourner commented Jul 23, 2018 •

edited

Loading

jfirebaugh commented Jul 23, 2018

mourner commented Jul 23, 2018

jfirebaugh commented Jul 23, 2018

hyperknot commented Jul 23, 2018 •

edited

Loading

mourner commented Aug 2, 2018

mourner commented Sep 24, 2018

hyperknot commented Sep 24, 2018 •

edited

Loading

mourner commented Sep 24, 2018

hyperknot commented Sep 24, 2018

mourner commented Oct 17, 2018

hyperknot commented Oct 17, 2018

hyperknot commented Oct 18, 2018

mourner commented Oct 18, 2018

hyperknot commented Oct 18, 2018 via email

hyperknot commented Oct 18, 2018 •

edited

Loading

kkaefer commented Oct 19, 2018

kkaefer commented Oct 19, 2018

mourner commented Nov 27, 2018

Avoid XHR when possible for faster raster tile loading #7008

Avoid XHR when possible for faster raster tile loading #7008

Conversation

mourner commented Jul 23, 2018 • edited Loading

Launch Checklist

hyperknot Jul 23, 2018

Choose a reason for hiding this comment

hyperknot Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

mourner Jul 23, 2018

Choose a reason for hiding this comment

mourner commented Jul 23, 2018

hyperknot commented Jul 23, 2018

jfirebaugh commented Jul 23, 2018

mourner commented Jul 23, 2018

hyperknot commented Jul 23, 2018 • edited Loading

jfirebaugh commented Jul 23, 2018

hyperknot commented Jul 23, 2018

jfirebaugh commented Jul 23, 2018

mourner commented Jul 23, 2018

jfirebaugh commented Jul 23, 2018

mourner commented Jul 23, 2018 • edited Loading

jfirebaugh commented Jul 23, 2018

mourner commented Jul 23, 2018

jfirebaugh commented Jul 23, 2018

hyperknot commented Jul 23, 2018 • edited Loading

mourner commented Aug 2, 2018

mourner commented Sep 24, 2018

hyperknot commented Sep 24, 2018 • edited Loading

mourner commented Sep 24, 2018

hyperknot commented Sep 24, 2018

mourner commented Oct 17, 2018

hyperknot commented Oct 17, 2018

hyperknot commented Oct 18, 2018

mourner commented Oct 18, 2018

hyperknot commented Oct 18, 2018 via email

hyperknot commented Oct 18, 2018 • edited Loading

kkaefer commented Oct 19, 2018

kkaefer commented Oct 19, 2018

mourner commented Nov 27, 2018

mourner commented Jul 23, 2018 •

edited

Loading

hyperknot Jul 23, 2018 •

edited

Loading

hyperknot commented Jul 23, 2018 •

edited

Loading

mourner commented Jul 23, 2018 •

edited

Loading

hyperknot commented Jul 23, 2018 •

edited

Loading

hyperknot commented Sep 24, 2018 •

edited

Loading

hyperknot commented Oct 18, 2018 •

edited

Loading