Optimize and simplify tile retention logic #6995

mourner · 2018-07-20T10:47:04Z

While investigating #6643, I noticed that our tile retention logic (and _findLoadedChildren use in particular) was accidentally quadratic. It didn't manifest with vector tiles because they're big and there are only 6–8 at the time, but with small raster tile layers, where there can be 40–80 tiles loading at the same time, it blows up.

This PR brings down children retention CPU time on a test fly animation from the #6643 case down from 812ms to 37ms (from quadratic to linear). This somewhat reduces jank overall (maybe 20%), but there's still a lot of remaining jank which is almost fully dominated by XHR image requests — I'll explore whether we can do something about it in another PR.

Commits in the PR are self-contained so it's easier to review them one by one, and let's rebase-merge, not squash.

Launch Checklist

briefly describe the changes in this PR
~~write tests for all new functionality~~ already covered
~~document any changes to public APIs~~
post benchmark scores
manually test the debug page

mourner · 2018-07-20T10:53:18Z

Paint benchmark is not affected because it doesn't exercise small tiles use case:

hyperknot · 2018-07-24T09:42:48Z

I've deployed to production with the 3 raster PRs included, and I'm getting a large number of "Cannot read property 'clearFadeHold' of undefined" exceptions, because of source_cache.js.

https://sentry.io/share/issue/0c5f7a31a923428ca24a43ad1705d14c/

mourner · 2018-07-24T09:48:39Z

@hyperknot this is likely a bug in #6995 — will investigate.

hyperknot · 2018-07-24T09:50:42Z

This is #6995 :-)

mourner · 2018-07-24T16:32:28Z

@hyperknot pushed a commit that might fix this, can you try it out?

mollymerp

nice catch @mourner! This looks good to me – just a few nits with the comments.

I'm pretty sure native's update_renderables algorithm doesn't have this accidentally quadratic problem because it doesn't use a findLoadedChildren equivalent, but it might be worth refactoring that algorithm to match the structure of this code to improve readability and comparability in the future.

mollymerp · 2018-07-24T20:32:28Z

src/source/source_cache.js

-        let found = false;
-
+    _retainLoadedChildren(
+        tiles: {[any]: OverscaledTileID},


can the any be replaced with number here?

also can the name of this argument be more descriptive like idealTiles or something else to avoid confusion with this._tiles?

can the any be replaced with number here?

Yes, but I'd leave this for a separate PR, since improving types for all objects that use tile IDs as keys is a pretty big change. Currently it's either any or string | number throughout the code because when you iterate with for (const id in obj), id is always a string, and converting it everywhere is cumbersome.

mollymerp · 2018-07-24T20:38:07Z

src/source/source_cache.js

+            // loop through ancestors of the topmost loaded child to see if there's one that needed it
+            let tileID = topmostLoadedID;
+            while (tileID.overscaledZ > zoom) {
+                tileID = tileID.scaledTo(tileID.overscaledZ - 1);


nit: can you use topmostLoadedID in the conditional and have tileID be a const?

while (topmostLoadedID.overscaledZ > zoom) { const tileID = topmostLoadedID.scaledTo(topmostLoadedID.overscaledZ-1); ...

No, since tileID loops through parents — changing to the above would do a different thing.

oh yeah 🤦‍♀️ its a while loop

mollymerp · 2018-07-24T20:41:07Z

src/source/source_cache.js

-                // If the drawRasterTile has never seen this tile, then
-                // tile.fadeEndTime may be unset.  In that case, or if
-                // fadeEndTime is in the future, then this tile is still
-                // fading in. Find tiles to cross-fade with it.


can we modify and keep this comment to explain the continue condition below?

I felt this would be redundant due to the comment after that. So one would say "skip tiles not loaded or already faded in", and another "do X with tiles that are loaded but still fading in".

mollymerp · 2018-07-24T20:43:07Z

src/source/source_cache.js

+            }
+        }
+
+        // retain any loaded children of ideal riles up to maxCoveringZoom


nit : retain any loaded children of missing ideal riles up to maxCoveringZoom

mollymerp · 2018-07-24T20:47:05Z

src/source/source_cache.js

-                        }
-                    }
+            // As we ascend up the tile pyramid of the ideal tile, we check whether the parent
+            // tile has been previously requested (and errored in this case due to the previous conditional)


the due to the previous conditional part of this comment no longer makes sense here – can change to and we can infer has errored or is loading because tiles with loaded data are skipped at the beginning of this block or something

mollymerp · 2018-07-24T20:53:32Z

src/source/source_cache.js

+            if (tile.hasData()) continue;
+
+            // The tile we require is not yet loaded or does not exist.
+            // We are now attempting to load a parent tile if children are not enough.


This second line comment can be deleted (parent tile loading doesn't start til L614 and is already commented down there

hyperknot · 2018-07-24T23:29:34Z

@mourner sure, I'll do a fresh build tomorrow and push it to production and wait a day to see if any errors are appearing.

hyperknot · 2018-07-26T15:58:04Z

OK, I deployed a build from bbbea02 and 323aaf0, I'll report about the results in a few days.

mourner · 2018-07-30T10:04:44Z

@hyperknot how's it going?

hyperknot · 2018-07-30T10:38:10Z

The new build is perfect so far, not a single error related to tiles.

jfirebaugh · 2018-07-30T20:33:17Z

src/source/source_cache.js

@@ -324,59 +324,64 @@ class SourceCache extends Evented {
    }

    /**
-     * Recursively find children of the given tile (up to maxCoveringZoom) that are already loaded;
-     * adds found tiles to retain object; returns true if any child is found.
+     * Retain children of the given set of tiles (up to maxCoveringZoom) that are already loaded;


Can this mention the zoom parameter too? It seems to be something like:

Retain children of the given set of tiles that are loaded and have a zoom between zoom (exclusive) and maxCoveringZoom (inclusive).

jfirebaugh · 2018-07-30T21:05:55Z

src/source/source_cache.js

                }
            }
        }
-        return found;
    }

    /**
     * Find a loaded parent of the given tile (up to minCoveringZoom);
     * adds the found tile to retain object and returns the tile if found


This clause no longer applies.

* clean up tile retention logic a bit * simplify retention logic further * simplify raster tile retaining logic * avoid delete when retaining child tiles * much faster children retention logic * simplify parent tile retention * address PR feedback * clear up source cache comments

mourner requested review from ansis and mollymerp July 20, 2018 10:47

This was referenced Jul 20, 2018

Raster tile loading blocking UI, FPS < 2.0 #6643

Closed

Avoid XHR when possible for faster raster tile loading #7008

Closed

mourner added 5 commits July 23, 2018 23:59

clean up tile retention logic a bit

5b4a9d9

simplify retention logic further

2bd0b65

simplify raster tile retaining logic

6a13aeb

avoid delete when retaining child tiles

0737b11

much faster children retention logic

45bbc09

mourner force-pushed the optimize-tile-retention branch from 1bee1cb to 45bbc09 Compare July 23, 2018 20:59

simplify parent tile retention

232b618

mollymerp approved these changes Jul 24, 2018

View reviewed changes

address PR feedback

323aaf0

mourner requested a review from jfirebaugh July 30, 2018 19:15

jfirebaugh reviewed Jul 30, 2018

View reviewed changes

mollymerp mentioned this pull request Jul 30, 2018

prevent overdraw by not retaining parent tiles for sparse tilesets #7051

Closed

6 tasks

clear up source cache comments

210b686

mourner merged commit 7da29b9 into master Jul 31, 2018

mourner deleted the optimize-tile-retention branch November 2, 2018 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize and simplify tile retention logic #6995

Optimize and simplify tile retention logic #6995

mourner commented Jul 20, 2018 •

edited

Loading

mourner commented Jul 20, 2018

hyperknot commented Jul 24, 2018 •

edited

Loading

mourner commented Jul 24, 2018

hyperknot commented Jul 24, 2018

mourner commented Jul 24, 2018

mollymerp left a comment

mollymerp Jul 24, 2018

mollymerp Jul 24, 2018

mourner Jul 25, 2018

mollymerp Jul 24, 2018

mourner Jul 24, 2018

mollymerp Jul 25, 2018

mollymerp Jul 24, 2018

mourner Jul 25, 2018 •

edited

Loading

mollymerp Jul 24, 2018

mollymerp Jul 24, 2018

mollymerp Jul 24, 2018

hyperknot commented Jul 24, 2018

hyperknot commented Jul 26, 2018

mourner commented Jul 30, 2018

hyperknot commented Jul 30, 2018

jfirebaugh Jul 30, 2018

jfirebaugh Jul 30, 2018

Optimize and simplify tile retention logic #6995

Optimize and simplify tile retention logic #6995

Conversation

mourner commented Jul 20, 2018 • edited Loading

Launch Checklist

mourner commented Jul 20, 2018

hyperknot commented Jul 24, 2018 • edited Loading

mourner commented Jul 24, 2018

hyperknot commented Jul 24, 2018

mourner commented Jul 24, 2018

mollymerp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mourner Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyperknot commented Jul 24, 2018

hyperknot commented Jul 26, 2018

mourner commented Jul 30, 2018

hyperknot commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mourner commented Jul 20, 2018 •

edited

Loading

hyperknot commented Jul 24, 2018 •

edited

Loading

mourner Jul 25, 2018 •

edited

Loading