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

Offline downloads - longer running transactions #11235

Closed
wants to merge 3 commits into from

Conversation

ivovandongen
Copy link
Contributor

@ivovandongen ivovandongen commented Feb 18, 2018

Fixes #11217
Closes #10252

Adds long(er) running transactions to offline downloads to avoid too much i/o overhead. Currently commits every 128 resources (very randomly picked, but performs well). Add-hoc cached resources and region (meta-data) are committed immediately and commit any region resources as well.

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
  • Current hardware: Pixel 2 XL
  • Old hardware: Galaxy Nexus


Before:

  • Current hardware: ~90 seconds

  • Old hardware: 15 minutes+



After

  • Current hardware: ~20 seconds
  • Old hardware: ~130 seconds

@ivovandongen ivovandongen self-assigned this Feb 18, 2018
@ivovandongen ivovandongen requested a review from kkaefer February 18, 2018 15:14
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.

Capturing a chat discussion with @ivovandongen about these changes:

The approach to transactions prior to this PR follows several rules that I think are important for being able to reason about database commits and rollback:

  • Transactions must be explicitly committed. In the event of an exception, stack unwinding will cause open transactions to roll back automatically.
  • Transactions follow the same nesting as the program stack. The interface intentionally makes it difficult to create transaction lifetimes that don't have a natural correspondence with stack frame lifetimes.
  • OfflineDatabase encapsulates the transactions necessary for maintaining offline database consistency.

This PR removes all of these rules. I think it'll make reasoning about offline database consistency -- both schema-level consistency, and consistency with in-memory state such as that held by OfflineDownload -- much more difficult. For instance:

  • I can't tell if OfflineDatabase still guarantees safety if two writers attempt an INSERT at the same moment.
  • I can't tell what will happen if an OfflineDownload updates completedResourceCount, completedResourceSize, etc., but then the transaction that inserted that resource is later rolled back.

We have seen (#7920) that database consistency is highly important, and when something goes wrong, costly to diagnose and fix. This change seems likely to increase the frequency and difficulty of consistency issues. I think we ought to explore different approaches instead:

@ivovandongen
Copy link
Contributor Author

An OfflineDatabase batch insert API

See #11284

WAL (#6334)

Experimentation shows no improvements when using WAL + synchronous on Android

@jfirebaugh jfirebaugh deleted the offline-download-transactions branch July 27, 2018 22:46
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.

2 participants