Request again when obtained data expires #2617

kkaefer · 2015-10-15T21:09:11Z

We're currently treating a request as a one-time thing. We should change this to have "Request" mean "monitor this resource and send me updates about it". This means that the file source needs to keep track of when the resource expires, and issue a new request once it expires. This solves the following cases:

Tiles with a very short TTL will display stale content, especially in situations where a map view is visible longer (e.g. an iPad in a kiosk mode)
We obtain a file from cache just before it expires
We can use the same infrastructure to implement Use cached tiles while doing a if-none-modified request #896

kkaefer · 2015-10-15T21:17:17Z

A few notes on the current code: Requests are now monitoring until they're explicitly cancelled. We're also returning expired content (a big part of what #896 is about!). However, none of the stale content, or updating requests are used in code yet, since we immediately return for stale responses, and cancel requests immediately after we got the first response.

Use updated data for re-parsing vector tiles. It's currently possible to just disable the cancellation, so they will get updates, however, there's still a slight flicker. Likely this is related to Prevent flickering when adding/removing annotations #1688.
Be smarter about not notifying requesters when the data didn't change: Requesters can store a std::shared_ptr<const std::string>, and compare it with the new one they receive. Doing so will not retain the data additionally since we already retain it in the FileSource.)
When marking a Response as stale, we should share its content to avoid copying (=> std::shared_ptr<const std::string>). Same goes for creating new Response objects for 304 HTTP responses.
Figure out what to do when servers respond with a max-age=0, or with an Expires time that is (consistently) in the past. Ideally we would treat this as expires = 0, and not schedule reloads until we get a new explicit request.

kkaefer · 2015-10-21T14:38:54Z

Here's a script to try this out:

Start the server with:

git clone git@gist.github.com:d74ed2b193bbd1a9507f.git tileserver
cd tileserver
npm install
node index.js

Apply this patch to mapbox-gl-native:

diff --git a/macosx/Info.plist b/macosx/Info.plist
index 514db11..f0e5c7f 100644
--- a/macosx/Info.plist
+++ b/macosx/Info.plist
@@ -32,6 +32,11 @@
     <string>0.0.1</string>
     <key>NSHighResolutionCapable</key>
     <true/>
+    <key>NSAppTransportSecurity</key>
+    <dict>
+        <key>NSAllowsArbitraryLoads</key>
+        <true/>
+    </dict>
     <key>CFBundleURLTypes</key>
     <array>
       <dict>
diff --git a/platform/default/default_styles.cpp b/platform/default/default_styles.cpp
index 9ee5414..2382e5c 100644
--- a/platform/default/default_styles.cpp
+++ b/platform/default/default_styles.cpp
@@ -4,6 +4,7 @@ namespace mbgl {
 namespace util {

 const std::vector<std::pair<std::string, std::string>> defaultStyles = {
+    { "http://localhost:3000/style.json", "Test" },
     { "asset://styles/streets-v8.json", "Mapbox Streets" },
     { "asset://styles/emerald-v8.json", "Emerald" },
     { "asset://styles/light-v8.json", "Light" },

Then, start the osxapp application and you will see this:

The server sends out tiles with a max-age=1, which contains a line from (0,0) to to the right bottom, with the length depending on the time.

kkaefer · 2015-10-21T15:41:16Z

/cc @jfirebaugh

jfirebaugh · 2015-10-22T01:11:39Z

src/mbgl/storage/request_holder.hpp

+
+class Request;
+
+class RequestHolder {


What's the reason for a separate RequestHolder class, rather than the usual pattern of returning a std::unique_ptr<Request> and having ~Request auto-cancel?

I implemented this at first, but backtracked because the pull request grew too big due to too many changes necessary for the unit tests. I'll try again.

Feel free to ticket and queue up for a later PR if you want.

ljbade · 2015-10-22T22:39:30Z

@kkaefer Did a quick test on Android. No problems found. And MapView is correctly GC'd.

👍

bleege · 2015-10-22T23:00:01Z

Looks good @kkaefer

jfirebaugh · 2015-10-22T23:15:50Z

src/mbgl/map/tile_worker.cpp

@@ -87,21 +113,15 @@ void TileWorker::parseLayer(const StyleLayer& layer, const GeometryTile& geometr
 return;
 }

- // This is a singular layer. Check if this bucket already exists.
- if (getBucket(layer))


What's happening here? I believe this check is what makes the layer ref mechanism efficient, by avoiding creating duplicate buckets for the referent layers.

That used to be the case, but the parsing is now split up into two distinct step: The initial bucket creation, and the actual parsing. We're no longer creating a new bucket just to throw away that object if we realize we don't have the resources yet. Instead, we're creating all bucket objects just once, then try to parse. If that doesn't work just yet, we're saving the bucket for later. All later "parse" steps will call parsePendingLayers, which just processes the buckets we saved for later.

Where does ref deduping happen, exactly? I'm still not seeing that.

Oops you're right. Will add that back.

This is now done before even calling parseLayer().

jfirebaugh · 2015-10-23T17:33:10Z

src/mbgl/map/vector_tile_data.cpp

+ if (angle == currentAngle && pitch == currentPitch && collisionDebug == currentCollisionDebug)
+ return;
+
+ if (workRequest) {


Shouldn't this check should go after the assignments to lastAngle, etc, so that when the callback calls redoPlacement again it uses the most recent values?

Yup, cleaned this up.

By not automatically destroying Request objects after the result has been delivered, we are making sure that we can potentially fire the callback multiple times without adverse effects. This means that you have to hold on to the result of fs->request(), can explicitly cancel it if you don't want to be notified of data changes anymore. Not doing so will monitor the request indefinitely and will prevent the app from exiting.

We're now returning stale responses from cache. Those responses will have the `stale` flag set to true. Currently, all requesters in the core code discard stale responses, and cancel the request immediately after they got a non-stale response.

When we get a request with an explicit expiration time, we're now starting a timer, and will trigger a refresh once the data expires. This gives requesters a chance to update their data.

It's not implemented in GCC 4.9.2's stdlib (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57250). Instead, we're now always using a mutex to protect access; we previously created a mutex only on cancelation, but since we're always canceling now, it makes sense to allocate it right away.

We're now reparsing tiles when they expire. We're also swapping out buckets atomically to avoid flickering data; i.e. we're displaying the old data as long as we don't have a new parsed bucket for that layer yet. The parsed buckets now live in the *TileData objects rather than in the TileWorker; only partially parsed == pending buckets will remain in the TileWorker. Once they're parsed, they're moved to the *TileData object.

Fixes an issue where updates to stale tiles would remove labels altogether until the map was rotated.

jfirebaugh · 2015-10-26T18:47:59Z

src/mbgl/map/tile_worker.cpp

- // the layer ordering needs to be respected when calculating text
- // collisions. Although, at this point, we requested all the resources
- // needed by this tile.
- if (partialParse) {


Does this change how partial parsing works? Previously, because partialParse was a member variable, if any symbol bucket needed dependencies, all subsequent symbol buckets would be treated as partial. Now it looks like only a bucket that needsDependencies is treated as partial. From my reading of the above comment, the previous behavior was intentional.

We can create new buckets in an arbitrary order; the only place where order matters is when calling SymbolBucket::placeFeatures. In the current scenario it's indeed possible that we're placing buckets in the wrong order. Will create a pull request that fixes this.

kkaefer added the refactor label Oct 14, 2015

kkaefer self-assigned this Oct 15, 2015

kkaefer added the in progress label Oct 15, 2015

kkaefer force-pushed the 2617-request-again-on-expiry branch 6 times, most recently from 3769781 to f522765 Compare October 21, 2015 14:25

kkaefer force-pushed the 2617-request-again-on-expiry branch from f522765 to eeebca7 Compare October 21, 2015 15:39

This was referenced Oct 21, 2015

Prevent flickering when adding/removing annotations #1688

Closed

Update layers/tiles while displaying #2481

Closed

kkaefer force-pushed the 2617-request-again-on-expiry branch from eeebca7 to b43b1e8 Compare October 21, 2015 20:01

jfirebaugh reviewed Oct 22, 2015
View reviewed changes

This was referenced Oct 22, 2015

Use cached tiles while doing a if-none-modified request #896

Closed

Use stale Glyph PBFs #2720

Closed

Use stale styles #2721

Merged

Use stale Sprites + JSON #2722

Closed

Use stale TileJSON data #2724

Closed

kkaefer added the performance Speed, stability, CPU usage, memory usage, or power usage label Oct 22, 2015

kkaefer force-pushed the 2617-request-again-on-expiry branch 2 times, most recently from 1c3b1b5 to c0de7cd Compare October 22, 2015 21:23

jfirebaugh reviewed Oct 22, 2015
View reviewed changes

kkaefer force-pushed the 2617-request-again-on-expiry branch from c0de7cd to 4ca70c9 Compare October 23, 2015 09:57

kkaefer force-pushed the 2617-request-again-on-expiry branch from 4ca70c9 to 2067b4b Compare October 23, 2015 13:19

jfirebaugh reviewed Oct 23, 2015
View reviewed changes

kkaefer added 7 commits October 26, 2015 15:54

[core] use RAII-style lifetime tracking of Request objects

096a3ed

[core] add support for stale responses

6a7334b

We're now returning stale responses from cache. Those responses will have the `stale` flag set to true. Currently, all requesters in the core code discard stale responses, and cancel the request immediately after they got a non-stale response.

[core] set a timer and auto-refresh requests when they expire

9bbd9ab

When we get a request with an explicit expiration time, we're now starting a timer, and will trigger a refresh once the data expires. This gives requesters a chance to update their data.

[core] Make response data shared to avoid excessive copying

5173bf1

kkaefer force-pushed the 2617-request-again-on-expiry branch from 2067b4b to a3a9478 Compare October 26, 2015 14:54

[core] always reparse with the freshest possible placement config

75dec6f

Fixes an issue where updates to stale tiles would remove labels altogether until the map was rotated.

kkaefer force-pushed the 2617-request-again-on-expiry branch from a3a9478 to 75dec6f Compare October 26, 2015 15:48

kkaefer merged commit 75dec6f into master Oct 26, 2015

kkaefer removed the in progress label Oct 26, 2015

kkaefer deleted the 2617-request-again-on-expiry branch October 26, 2015 16:20

This was referenced Oct 26, 2015

Crash in HTTPRequest::handleResponse() #1837

Closed

Crashes in http_request_nsurl.mm #2417

Closed

1ec5 mentioned this pull request Oct 26, 2015

CFNetwork crash on fast changing or poor Reachability #2793

Closed

friedbunny mentioned this pull request Oct 26, 2015

FileSource HTTPNSURLRequest::start() crash #2123

Merged

jfirebaugh reviewed Oct 26, 2015
View reviewed changes

jfirebaugh mentioned this pull request Oct 26, 2015

rendering tests: Error: timed out after 20 seconds #2808

Closed

friedbunny mentioned this pull request Oct 27, 2015

Crashes when removing and adding annotations on regionDidChangeAnimated #2681

Closed

kkaefer mentioned this pull request Oct 27, 2015

Place symbols in correct order #2820

Closed

ljbade mentioned this pull request Oct 29, 2015

Try OkHTTP on Android again #2856

Closed

This was referenced Jan 19, 2016

Vector tiles for dynamic data mapbox/mapbox-gl-js#1946

Closed

Request resources again when they expire per their HTTP headers mapbox/mapbox-gl-js#1947

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request again when obtained data expires #2617

Request again when obtained data expires #2617

kkaefer commented Oct 15, 2015

kkaefer commented Oct 15, 2015

kkaefer commented Oct 21, 2015

kkaefer commented Oct 21, 2015

jfirebaugh Oct 22, 2015

kkaefer Oct 22, 2015

jfirebaugh Oct 22, 2015

ljbade commented Oct 22, 2015

bleege commented Oct 22, 2015

jfirebaugh Oct 22, 2015

kkaefer Oct 23, 2015

jfirebaugh Oct 23, 2015

kkaefer Oct 26, 2015

kkaefer Oct 26, 2015

jfirebaugh Oct 23, 2015

kkaefer Oct 26, 2015

jfirebaugh Oct 26, 2015

kkaefer Oct 27, 2015

kkaefer Oct 27, 2015


		class Request;

		class RequestHolder {

Request again when obtained data expires #2617

Request again when obtained data expires #2617

Conversation

kkaefer commented Oct 15, 2015

kkaefer commented Oct 15, 2015

kkaefer commented Oct 21, 2015

kkaefer commented Oct 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljbade commented Oct 22, 2015

bleege commented Oct 22, 2015

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment