Re-request expired resources #3944

lbud · 2017-01-10T20:36:08Z

Closes #1947.

This PR modifies ajax#getArrayBuffer to request Cache-Control and Expires headers. SourceCache maintains timers for tiles with set expiry dates for both active and cached tiles:

when an active tile's timer expires, it re-requests the tile
when a cached tile's timer expires, it is removed from the cache

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
manually test the debug page

lbud · 2017-01-24T00:19:20Z

ready for 👀 🚦

anandthakker · 2017-01-24T15:53:33Z

js/source/source_cache.js

+
+                if (this._cacheTimers[coord.id]) {
+                    clearTimeout(this._cacheTimers[coord.id]);
+                    delete this._cacheTimers[coord.id];


Even though delete is clearer, this might be a case where it's better to use this._cacheTimers[coord.id] = undefined, because this method is (relatively) hot and, as I understand it, delete causes V8 to re-compute an object's hidden class.

Oo @anandthakker thank you — did not know this, googled a little hole for myself to understand further: googleapis/google-api-nodejs-client#375

anandthakker · 2017-01-24T15:58:33Z

js/source/source_cache.js

@@ -396,14 +411,19 @@ class SourceCache extends Evented {
            tile = this._cache.get(wrapped.id);
            if (tile) {
                tile.redoPlacement(this._source);
+                if (this._cacheTimers[wrapped.id]) {
+                    clearTimeout(this._cacheTimers[wrapped.id]);
+                    delete this._cacheTimers[wrapped.id];


See above re: delete

anandthakker · 2017-01-24T16:04:52Z

js/source/source_cache.js

+        if (tileExpires) {
+            this._cacheTimers[id] = setTimeout(() => {
+                this._cache.remove(id);
+                delete this._timers[id];


should this be this._cacheTimers[id]?

it should, yes 🙇‍♀️

anandthakker · 2017-01-24T16:15:18Z

js/util/lru_cache.js

+    remove(key: string) {
+        if (!this.has(key)) { return this; }
+
+        this.onRemove(this.data[key]);


In add(), it looks like this.onRemove() gets called after the item is deleted from this.data, whereas here, it's getting called before; being inconsistent could cause problem at some point.

Also, a question: am I missing something, or does cache.get() actually also remove the item? If that's so, is remove() redundant? (Although, admittedly, I normally would not expect a method called get() to have a side effect...)

cache.get removes an item from the cache and returns it, so there is some repeated code here, and we could use cache.get to delete tiles from the cache, but then it's returning expiring tiles to nowhere and would be less intuitive in code (when cached tile's timer expires, retrieve it from the cache … vs when cached tile's timer expires, remove it from the cache). 😕

Yeah, fair enough. I'd actually be okay with "returning expired tiles to nowhere" (i.e. the calling code need not actually store the return value), BUT because it's called get I agree it would look pretty weird:

// oh, we need to remove the cache entry now, so cache.get(id) // cool, moving on

I sort of wonder if we should just rename LRUCache#get to LRUCache#remove -- that way, whether the calling code actually uses the return value or not, it is very clear that you're removing the entry from the cache.

anandthakker · 2017-01-24T16:31:57Z

js/source/source_cache.js

+                    clearTimeout(this._cacheTimers[coord.id]);
+                    delete this._cacheTimers[coord.id];
+                    this._setTimers(coord.id, cachedTile);
+                }


It looks to me like we always do addTile() with any nonempty return value from this method -- since that's the case, and since we have equivalent logic for removing cacheTimer / adding timer already in addTile, is it possible that we could avoid redundancy by only doing it in addTile?

anandthakker · 2017-01-24T20:32:06Z

@lbud changes LGTM!

All I have left are a couple nits on naming/clarity that I should have thought of earlier:

LRUCache get vs remove see comment here
In SourceCache, what do you think about renaming _setTimers to something like _setTileReloadTimer and _setCacheTimers to _setCacheRemovalTimer?
I'm less sure of this than the above, but: would it make sense to move the this._cacheTimers[id] = undefined into _setTileReloadTimer() (and similar for this._timers = undefined), so that the calling code could be cleaner (and so that each tile having <= 1 timer going is enforced by those functions)?

^ this is all motivated by the notion that caching logic in general and SourceCache in particular tend to be hard to read/reason about and easy to mess up later (for me, anyway 😕 )

anandthakker

@lbud Marking as approved since my remaining comments are all opinion/judgment calls

lbud · 2017-01-24T21:41:26Z

LRUCache get vs remove see comment here

I'd be okay with renaming it, although I instinctively think it's fine to keep both: adding remove doesn't add much code complexity, and they exist to serve different purposes. I'm going to leave it for now and if we feel strongly we can revise in the future.

I'm less sure of this than the above, but: would it make sense to move the this._cacheTimers[id] = undefined into _setTileReloadTimer() (and similar for this._timers = undefined), so that the calling code could be cleaner (and so that each tile having <= 1 timer going is enforced by those functions)?

I started doing this, but hesitated because of this part of removeTile — to be honest I'm not quite sure in which cases a tile would land here and return early, but I'm not comfortable reordering things in this fn at the moment (which combining timer resetting with cacheTimer setting would necessitate). 🤔

* set cache timers in SourceCache + remove tiles from the cache when they expire * handle for setting/resetting timers when moving from SourceCache <-> LRUCache

* Avoid 'delete's so as to not re-compute hidden classes * Remove redundant cache timer clearing logic from 'findLoadedParent' * Consistify lru_cache's order of operations re deleting

* _setTimers -> _setTileReloadTimer * _setCacheTimer -> _setCacheInvalidationTimer

anandthakker · 2017-01-24T21:43:59Z

I started doing this, but hesitated because of this part of removeTile — to be honest I'm not quite sure in which cases a tile would land here and return early, but I'm not comfortable reordering things in this fn at the moment (which combining timer resetting with cacheTimer setting would necessitate)

Ah, yeah I see what you mean. 👍

cammanderson · 2017-01-26T02:23:47Z

Very cool stuff - excited about this. We have a use case for this right now, great to see it is in action!

karlguillotte · 2017-02-07T03:50:12Z

Since I upgraded to the version that include that feature, I started getting bunch of errors Unable to get property 'state' of undefined or null reference.

It is coming from

mapbox-gl-js/src/source/source_cache.js

Line 149 in b9e10b9

if (tile.state !== 'loading') {

.

I am sorry, I can not point you out to any gist that shows the errors. I am still investigating. Wondering if someone has that same issue?

I am using the new smart setStyle().

Thanks for MapBox GL.

anandthakker · 2017-02-07T13:51:46Z

@karlguillotte thank you for reporting this. Could you please open a new issue describing this bug?

I know you said that you're still investigating -- if you're able to provide a minimal example demonstrating the problem, that would greatly improve our chances of to diagnosing and solving it.

AndyMoreland · 2017-02-08T00:00:46Z

I have this bug also. Working on tracking down exactttllyy what's going on.

AndyMoreland · 2017-02-08T00:24:47Z

The root cause appears to be this line: https://github.com/mapbox/mapbox-gl-js/blob/master/src/source/source_cache.js#L438

The issue occurs when tile.getExpiry() - (new Date()).getTime() is larger than a 32-bit signed int. In my case the delta is 2592000000 and the timeout in executes immediately which results in an infinite loop of reloading tiles and the errors that @karlguillotte reported if I pan.

(Problem seems to be because of the implementation of setTimeout in browser JS engines, and in node: nodejs/node-v0.x-archive#8656)

lbud force-pushed the maxage branch from 26073dc to 4273e76 Compare January 10, 2017 21:11

lucaswoj added the under development label Jan 17, 2017

lbud force-pushed the maxage branch from e1bc72b to cbfc8ad Compare January 20, 2017 19:54

lbud changed the title ~~[not ready] Re-request expired resources~~ Re-request expired resources Jan 24, 2017

lbud force-pushed the maxage branch from bae3dc9 to bdc4dbe Compare January 24, 2017 00:07

lbud requested a review from lucaswoj January 24, 2017 00:18

lbud removed the under development label Jan 24, 2017

anandthakker reviewed Jan 24, 2017

View reviewed changes

anandthakker approved these changes Jan 24, 2017

View reviewed changes

aaronlidman and others added 15 commits January 24, 2017 13:41

Add cache control to vector tile source

cd80e00

Update getArrayBuffer for new response

3c3995f

Initial support for expiring cached tiles (needs tests etc)

956b98c

Remove clearExpiredTiles

166bd13

Cache handling:

2e1ec43

* set cache timers in SourceCache + remove tiles from the cache when they expire * handle for setting/resetting timers when moving from SourceCache <-> LRUCache

Flow annotations for new util fn (parseCacheControl)

947c8fe

Stub tile.setExpiryData for existing vector_tile_source test

50e04a0

Modify ajax.getImage to reflect ajax.getArrayBuffer changes

ef45448

Modify test implementation to reflect new getArrayBuffer response

af83348

Start adding tests (SourceCache, Tile, util.parseCacheControl)

0932d53

Set expiry data for raster tiles as well

bbe68ec

Remove s-maxage references

3c1b4b7

Rm s-maxage tests

1764c26

Address PR review comments:

39bc6ba

* Avoid 'delete's so as to not re-compute hidden classes * Remove redundant cache timer clearing logic from 'findLoadedParent' * Consistify lru_cache's order of operations re deleting

Renames:

a7f0e95

* _setTimers -> _setTileReloadTimer * _setCacheTimer -> _setCacheInvalidationTimer

lbud force-pushed the maxage branch from 2efe2bd to a7f0e95 Compare January 24, 2017 21:51

lbud merged commit 67fc7df into master Jan 24, 2017

lbud deleted the maxage branch January 24, 2017 22:21

averas mentioned this pull request Jan 27, 2017

Allow developers to clear all tile caches #2633

Closed

tobinbradley mentioned this pull request Feb 10, 2017

fix for re-request expired resources causing infinite loop #4255

Merged

chiatt mentioned this pull request Feb 14, 2017

Upgrade to mapbox-gl-js 0.32.1 archesproject/arches#1515

Closed

mourner mentioned this pull request Feb 28, 2017

Tiles flicker in Microsoft Edge #4252

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-request expired resources #3944

Re-request expired resources #3944

lbud commented Jan 10, 2017 •

edited

Loading

lbud commented Jan 24, 2017

anandthakker Jan 24, 2017

lbud Jan 24, 2017

anandthakker Jan 24, 2017

anandthakker Jan 24, 2017

lbud Jan 24, 2017

anandthakker Jan 24, 2017

lbud Jan 24, 2017

anandthakker Jan 24, 2017

anandthakker Jan 24, 2017 •

edited

Loading

anandthakker commented Jan 24, 2017 •

edited

Loading

anandthakker left a comment

lbud commented Jan 24, 2017

anandthakker commented Jan 24, 2017

cammanderson commented Jan 26, 2017

karlguillotte commented Feb 7, 2017 •

edited

Loading

anandthakker commented Feb 7, 2017

AndyMoreland commented Feb 8, 2017

AndyMoreland commented Feb 8, 2017 •

edited

Loading

Re-request expired resources #3944

Re-request expired resources #3944

Conversation

lbud commented Jan 10, 2017 • edited Loading

Launch Checklist

lbud commented Jan 24, 2017

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

anandthakker Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

anandthakker commented Jan 24, 2017 • edited Loading

anandthakker left a comment

Choose a reason for hiding this comment

lbud commented Jan 24, 2017

anandthakker commented Jan 24, 2017

cammanderson commented Jan 26, 2017

karlguillotte commented Feb 7, 2017 • edited Loading

anandthakker commented Feb 7, 2017

AndyMoreland commented Feb 8, 2017

AndyMoreland commented Feb 8, 2017 • edited Loading

lbud commented Jan 10, 2017 •

edited

Loading

anandthakker Jan 24, 2017 •

edited

Loading

anandthakker commented Jan 24, 2017 •

edited

Loading

karlguillotte commented Feb 7, 2017 •

edited

Loading

AndyMoreland commented Feb 8, 2017 •

edited

Loading