1360519 - Optimize memory usage of SQLiteHistory.getModifiedHistoryToUpload

Status: RESOLVED FIXED

(Firefox for iOS :: Sync, enhancement, P1)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

This bug is for follow up work to:

  Bug 1358780 - Crash in SQLiteHistory.getModifiedHistoryToUpload (OOM)

In that bug we did a workaround to get past the crashes. But we need a better solution that can process larger batches of modified history.

We initially only talked about optimizing getModifiedHistoryToUpload to use less memory, but I think we should also consider making this handle the chunked uploading of modified records in HistorySynchronizer.uploadModifiedPlaces().

(This only makes sense if there is a real possibility that 10s of thousands of modified history items have to be uploaded. And if a limit on getModifiedHistoryToUpload is not something we want.)

Comment 1

[Richard Newman [:rnewman]]

Note that uploadModifiedPlaces already chunks: it splits an incoming list of arbitrary size into chunks of 1000, producing HistoryPayloads (and thus JSON) immediately prior to upload.


Copying across my suggestion from Bug 1358780:

--
The likely culprit is the visit aggregation query:

        let historyLimit =
        "SELECT * FROM history " +
        "WHERE history.should_upload = 1 LIMIT ?"

        let sql =
        "SELECT " +
        "history.id AS siteID, history.guid AS guid, history.url AS url, history.title AS title, " +
        "v1.siteID AS siteID, v1.date AS visitDate, v1.type AS visitType " +
        "FROM " +
        "visits AS v1 " +
        "JOIN (\(historyLimit)) as history ON history.id = v1.siteID AND v1.type <> 0 " +
        "LEFT OUTER JOIN " +
        "visits AS v2 " +
        "ON v1.siteID = v2.siteID AND v1.date < v2.date " +
        "GROUP BY v1.date " +
        "HAVING COUNT(*) < ? " +
        "ORDER BY v1.siteID, v1.date DESC"

This query is tabular, so we'll accrue the URL and title up to 20 times for each history item.

The fix will be to split this into two queries. We'll find the history items we care about:

        let historyItems =
        "SELECT id, guid, url, title FROM history " +
        "WHERE should_upload = 1 LIMIT ?"

Run that, and collect the rows (`places[id] = Place(guid: guid, url: url, title: title)`) and IDs.

The cursor is dropped at this point, and only contains `limit` rows anyway.

Now use the IDs to retrieve visits, very similar to how we do it now (untested SQL):

        let historyIDs = "(123, 456, 789)"         // Construct this!
        let visitsQuery =
        "SELECT " +
        "v1.siteID AS siteID, v1.date AS visitDate, v1.type AS visitType " +
        "FROM " +
        "(SELECT * FROM visits WHERE siteID IN \(historyIDs) AND type <> 0) AS v1 " +
        "LEFT OUTER JOIN " +
        "visits AS v2 " +
        "ON v1.siteID = v2.siteID AND v1.date < v2.date " +
        "GROUP BY v1.date " +
        "HAVING COUNT(*) < ? " +
        "ORDER BY v1.siteID, v1.date DESC"

The results will be visits only, and we can accumulate them just as we do now, using the `siteID` to find the place.

Comment 3

[Stephan Leroux [:sleroux]]

Attachment: Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/2693

Comment 4

[Stephan Leroux [:sleroux]]

master 1b9bfcd7d8a838b9a0681c2ac5887958244684cc