Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

fixes #67: cancel unnecessary tile fetches #76

Merged
merged 15 commits into from
Feb 28, 2014
Merged

fixes #67: cancel unnecessary tile fetches #76

merged 15 commits into from
Feb 28, 2014

Conversation

incanus
Copy link
Contributor

@incanus incanus commented Feb 20, 2014

This addresses performance problems caused by waiting for tile fetches to complete even when the tiles will be discarded due to no longer being on screen. We currently discard the tiles, but not until after their network calls have played out.

The approach here is to cancel in-progress tile fetches for zoom levels not equal to the latest requested tile zoom.

  • Leans on determine supported OS versions #75 and replaces use of NSURLConnection with iOS 7 / OS X 10.9's new NSURLSession. This provides for cancelable, asynchronous, block-based network fetches without external dependencies, custom networking classes, or the complicated use of the delegate pattern in C++.
  • Adds platform::request_http_tile() which adds a tile object pointer argument. This allows the platform-specific code to inspect Tile::ID to get the zoom in question, as opposed to only being privy to the requested URL string. This avoids the need to know/guess that a tile resource is indeed what is requested or to parse its URL string to determine the zoom.
  • Does not call background_function() nor foreground_callback() arguments in the event of a tile cancellation in order to avoid tile error messages / potential complication with future tile 404 fallback mask level #74 fallback fetching due to HTTP 404.
  • Simplifies the iOS network activity UI code using NSURLSession's ability to query all pending tasks from a singleton.
  • Only posts a "needs render" notification in the event of a non-error tile completion.

/cc @springmeyer who helped me a bit with the tile object pointer argument solution.

Before video: https://dl.dropboxusercontent.com/u/575564/before_cancel.mov

After video: https://dl.dropboxusercontent.com/u/575564/after_cancel.mov

Gist of nearly 400 tile downloads in 30 seconds that were cancelled from second video run: https://gist.github.com/incanus/bc9fd387707b27efe289

@incanus
Copy link
Contributor Author

incanus commented Feb 21, 2014

An alternative, enhanced approach to this that occurred to me is making a platform::cancel_http_request() that takes a URL string argument. Then, the core engine could determine cancellation policy and send cancels off to the platform. But I think this could be a lot messier and bug-prone.

@kkaefer
Copy link
Member

kkaefer commented Feb 21, 2014

Cool! Any reason why tiles and non-tiles have a different API?

@ansis
Copy link
Contributor

ansis commented Feb 21, 2014

The approach here is to cancel in-progress tile fetches for zoom levels not equal to the latest requested tile zoom.

For the webgl version we also fetch a couple tiles at much lower zoom levels ,so that when panning and zooming it can really overscale those tiles instead of showing a blank areas. That wouldn't work with this approach to cancelling requests. That shouldn't block merging this improvement, but something to keep in mind for later.

@kkaefer
Copy link
Member

kkaefer commented Feb 21, 2014

This seems to cancel tile download request based on the latest zoom level. However, we actually now when we can cancel a tile load, there is a cancel() method an Tile that gets called if the tile is removed from the viewport. It's state is then set to obsolete and it'll be deleted once all references are gone. Currently, we cannot abort HTTP requests, but we do not parse tiles that are marked as obsolete. I think the way we should do this is to use some sort of future idiom, meaning the http_request() method returns a request object that we store in the tile, and that we can attach request handlers to. If we know we can cancel a HTTP request, we just use that object and the platform can decide what to do with it.

This would save us from future issues where we'd download tiles of other zoom levels that get cancelled by this magic algorithm, like @ansis described.

@incanus
Copy link
Contributor Author

incanus commented Feb 21, 2014

Yeah @ansis @kkaefer this is my main concern with the naivety of this approach (as well as with #74), and I was aware of the parsing abort already. Something along the lines of the future idiom you mention does sound good, so I'll spend some time working on that before we consider a merge here. It does make sense to push this down to the core rather than the platform if possible cleanly.

@incanus
Copy link
Contributor Author

incanus commented Feb 26, 2014

Ok, have a look at things now. This is all pushed down to the renderer level and happily, it is already managing Tile::state for us, so this does reduce a lot of platform-specific code.

  • Tile state ready is now called loaded.
  • There is a new state, parsed, to distinguish between network-fetched and data-parsed.
  • platform::request returns a Response struct that can be used to cancel a request at a later time. This plays particularly well with NSURLSession and its unique allotment of taskIdentifier and cancelable requests, but this should be portable.
  • Reminder that this still depends on determine supported OS versions #75 iOS 7+ / OS X 10.9+.

@incanus
Copy link
Contributor Author

incanus commented Feb 28, 2014

Quick look at this @kkaefer? I'd like to merge tomorrow.

@incanus
Copy link
Contributor Author

incanus commented Feb 28, 2014

This is working great in all of my testing. In fact, I keep merging it into my testing branches because the performance is much better.

incanus added a commit that referenced this pull request Feb 28, 2014
fixes #67: cancel unnecessary tile fetches
@incanus incanus merged commit a31fab1 into master Feb 28, 2014
@kkaefer
Copy link
Member

kkaefer commented Mar 1, 2014

Reason I didn't merge this yet is that linux support is still missing. I'm working on it and try to get it merged today or tomorrow.

@kkaefer
Copy link
Member

kkaefer commented Mar 1, 2014

oh, btw, it definitely works a lot better. Great job on the background URL fetches 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants