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

Offline download batches #11284

Merged
merged 2 commits into from
Jun 4, 2018
Merged

Offline download batches #11284

merged 2 commits into from
Jun 4, 2018

Conversation

ivovandongen
Copy link
Contributor

Fixes #11217
Closes #10252

Alternative to #11235 using batching of resource update/inserts.

Somewhat slower: +/- 30 seconds on Pixel XL 2, 130 seconds on Galaxy Nexus. Higher memory consumption; had to keep the batch size relatively small to avoid running out of memory on old devices.

@kkaefer kkaefer added the Core The cross-platform C++ core, aka mbgl label Feb 23, 2018
@ivovandongen ivovandongen force-pushed the offline-download-batches branch 2 times, most recently from 0790725 to 7c09f27 Compare March 7, 2018 12:41
@ivovandongen ivovandongen self-assigned this Mar 7, 2018
@kkaefer kkaefer requested a review from jfirebaugh March 15, 2018 17:33
@@ -136,6 +136,12 @@ mapbox::sqlite::Statement& OfflineDatabase::getStatement(const char* sql) {
return *it->second;
}

void OfflineDatabase::batch(BatchedFn&& fn) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of exposing a generic function like this, can we pass a list/collection of Resource/Response pairs and handle iteration over those, and transaction management internally? E.g. have a public putRegionResources() function that internally calls putRegionResource for every item, wrapping everything in a transaction.

@jfirebaugh
Copy link
Contributor

I just discovered that you can REPLACE in a way that avoids generating a new id:

REPLACE INTO resources (url, ..., id)
VALUES                 (?1,  ..., (SELECT id FROM resources WHERE url = ?1))

This takes advantage of the fact that id is the ROWID column, which will replace a NULL value with an autogenerated value on inserts.

However, there does not appear to be a way of determining whether such a statement replaced or inserted, which putResource / putTile needs for the return value. 😢If there were, we could avoid the transaction entirely.

@jfirebaugh
Copy link
Contributor

@ivovandongen Can you benchmark this approach versus https://github.com/mapbox/mapbox-gl-native/compare/no-transaction? I'm not sure if small automatic deferred transactions, or batched immediate transactions will be faster.

@ivovandongen
Copy link
Contributor Author

@ivovandongen Can you benchmark this approach versus https://github.com/mapbox/mapbox-gl-native/compare/no-transaction? I'm not sure if small automatic deferred transactions, or batched immediate transactions will be faster.

@jfirebaugh Between 95 - 100 seconds on the Pixel XL 2. The Galaxy Nexus is still charging, but I don't think it will be much better I'm afraid.

@jfirebaugh
Copy link
Contributor

jfirebaugh commented Mar 16, 2018

I'm not clear how 95-100 seconds compares. Can you summarize the results from the three approaches (master, no-transation, offline-download-batches)?

@ivovandongen
Copy link
Contributor Author

Pixel 2 Galaxy Nexus
master ~90 seconds
 15 minutes
no transaction 95 - 100 seconds not tested
offline-download-batches ~30 seconds ~130 seconds
longer running transactions ~20 seconds ~130 seconds

Tested with


  • Berlin region: 5195 tiles, north:52.6780473464 east:13.7603759766 south:52.3305137868 west:13.0627441406 max-zoom:15

  • Warmed up cache on the edge location

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great -- I think that offline-download-batches is the best balance between improved performance and code complexity.

@ivovandongen ivovandongen merged commit d928908 into master Jun 4, 2018
@ivovandongen ivovandongen deleted the offline-download-batches branch June 4, 2018 09:09
@kkaefer
Copy link
Contributor

kkaefer commented Jun 4, 2018

\o/

@friedbunny friedbunny added needs changelog Indicates PR needs a changelog entry prior to merging. and removed needs changelog Indicates PR needs a changelog entry prior to merging. labels Jun 5, 2018
@friedbunny
Copy link
Contributor

iOS changelog added in #12086.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants