616265 - Short-term efficiency improvements for large downloads on mobile

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

(Forked from Bug 615284.)

First syncs on Fennec can be problematic: too much memory is consumed, and too many GET requests are needed. OOM kills the process, and the first sync never completes.

(It completes on devices with lots of memory, but takes a long time.)

There are long-term improvements that we'd like to do for this; I'm using Bug 615284 to track these.

We're also waiting for places work to land, which will reduce memory usage and allow us to back out some of these changes.

This bug is for the following short-term work:

* Bump batch size: reduce number of requests needed for a sync.
* Cap the number of records fetched per sync (to 5,000 for now).
* Reduce the scope of a Places batch to that of a mobile batched fetch, which will reduce memory usage.

Comment 1

[Richard Newman [:rnewman]]

13:32:11 <@mconnor> @sdwilsh> mconnor: so, you could force a flush by getting a handle to nsPlacesDBFlush, QIing it to nsINavBookmarkObserver, and then calling onEndUpdateBatch, and then again onBeginUpdateBatch

Comment 2

[Richard Newman [:rnewman]]

Attachment: Add batching to History engine.

First cut at this. I verified that batching takes place, and doesn't error on either 3.6.12 XULrunner or Minefield xpcshell-tests, but I don't know if it does any good yet.

Thoughts welcome.

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 494859 [details] [diff] [review]
Add batching to History engine.

Some drive by feedback :)

>+var historyURL = "resource://gre/components/nsPlacesDBFlush.js";
>+var places = {};
>+var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
>+                       .getService(Components.interfaces.mozIJSSubScriptLoader);
>+loader.loadSubScript(historyURL, places);
>+
>+

...

>-  _sync: Utils.batchSync("History", SyncEngine),
>+  _flusher: null,
>+  _flushCount: 0,
>+  _prepareFlusher: function _prepareFlusher() {
>+    // Apparently this is supposed to be QIed as an nsINavBookmarkObserver,
>+    // but I don't see a way to get hold of the right class object to use
>+    // createInstance. This way appears to work...
>+    let flush = new places.nsPlacesDBFlush();

Yuck :) You want

  flush = Components.classesByID[Components.ID("c1751cfc-e8f1-4ade-b0bb-f74edfb8ef6a")]
                    .getService(Ci.nsINavBookmarkObserver);

(By QIing sdwilsh meant flush = flush.QueryInterface(...), but not necessary here with the getService call.)

>diff --git a/services/sync/tests/unit/test_syncengine_sync.js b/services/sync/tests/unit/test_syncengine_sync.js

I think these tests should go into a separate test_history_engine.js file.

Comment 4

[Richard Newman [:rnewman]]

Attachment: Add batching to History engine. v2

Philipp filled in a gap in my knowledge, so here's a better patch.

Comment 5

[Richard Newman [:rnewman]]

Attachment: Add batching to History engine. v3

… and with tests appropriately rejiggered. 90 passes!

Comment 6

[Richard Newman [:rnewman]]

Re bumping the batch size:

The batch size is currently 50, with a note that it's limited because of URL length restrictions. GUIDs can be up to 64 chars long.

Philipp's bookmark GUID work will drop that to 12 chars, which should allow us to go north of 200.

For mconnor's 17,000 history items, that would drop us from 340 GETs to 85: a far more pleasing number. (Adding a cap of 5,000 records would put the max number of fetches at 25, of course.)

Of course, the payload for each request would be four times the size...

mconnor, any thoughts on this?

The patch is trivial, so nothing attached.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Enable test_history_engine everywhere.

Comment 8

[Richard Newman [:rnewman]]

Attachment: Add download limit for history, lift magic number. v1

Add a downloadLimit property to engines. History is set to 5000.
    
Also lift the magic constant for history upload into constants.js.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Add download limit for history, lift magic number. v2

More sophisticated handling (e.g., server-sort), and only apply to mobile devices -- we don't want the hit of sorting for every user.

Test.

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 494887 [details] [diff] [review]
Enable test_history_engine everywhere.

Please make sure to file a bug on not doing this extra work once temp tables are gone.  We already have the _hasTempTables getter we could use, but that's always true for all current versions.

Comment 11

[Richard Newman [:rnewman]]

(In reply to comment #10)

> Please make sure to file a bug on not doing this extra work once temp tables
> are gone.  We already have the _hasTempTables getter we could use, but that's
> always true for all current versions.

Bug 616556.

Re evaluating this change: I plan to migrate my 30,000-history profile to storage version 4 this afternoon, then try three different Fennec first syncs (control, then one for each patch). That should give us a good idea of how much improvement we can expect, if any.

I will report back.

Comment 12

[Richard Newman [:rnewman]]

Try build kicked off with batching patch, based on current sync-central repo:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=62f9120345ff

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 494921 [details] [diff] [review]
Add download limit for history, lift magic number. v2

>+      // Sort and limit so that on mobile we only get the last X records.
>+      guidColl.limit = fetchLimit;
>+      guidColl.sort  = guidSort;

You can just set this directly to "index" here, since this is a mobile-only path (unless handled.length == Infinity), and save the previous step.  We really don't need guidSort except in this specific case.

Comment 14

[Richard Newman [:rnewman]]

dougt pointed me to this method for soft-limiting the sqlite heap:

http://people.mozilla.org/~dougt/sqlite_soft_limit_heap

We'd need a JS equivalent, of course, but it's a possibility for the future.

Comment 15

[Richard Newman [:rnewman]]

My current running sync has mapped 218,103,808. Memory in use is no lower than the Fennec build that preceded it. (See Bug 615167.)

Sync is still running.

It looks rather like flushing the places cache does almost nothing for our memory usage.

I forgot (again) to turn logging on before starting this sync, so I'll do another to verify that flushes are occurring… but if they are, we're going to need to look elsewhere for the memory hungry culprit.

Comment 16

[Philipp von Weitershausen [:philikon]]

When you rerun the test, please make sure you use a clean profile, not just Reset Sync. It makes a difference whether Sync has been used on a profile before and then reset or whether it's a brand new profile, mostly because there are no existing places annotations to reuse so those have to be created. This could make a difference.

Comment 17

[Richard Newman [:rnewman]]

(In reply to comment #16)
> When you rerun the test, please make sure you use a clean profile, not just
> Reset Sync. It makes a difference whether Sync has been used on a profile
> before and then reset or whether it's a brand new profile, mostly because there
> are no existing places annotations to reuse so those have to be created. This
> could make a difference.

I actually used the Android app manager to wipe all data and uninstall. Clean as a whistle. (Had to type everything in again, too, which makes me beg for J-PAKE.)

Comment 18

[Mike Connor [:mconnor]]

We should get these landed in fx-sync, and punt the batching entirely, since temp tables are going away post b8.

Comment 19

[Richard Newman [:rnewman]]

(In reply to comment #18)
> We should get these landed in fx-sync, and punt the batching entirely, since
> temp tables are going away post b8.

Do you mean "punt the flushing entirely" -- i.e., I should land the history limit?

Or "punt the batch size increase" -- i.e., I should land these two changes?

Or something else?

Comment 20

[Mike Connor [:mconnor]]

punt the flushing entirely.  We should still do the history limit, IMO, but let's actually leave the batch size until after b8.  Split that to a new bug?

Comment 21

[Richard Newman [:rnewman]]

Attachment: Add download limit for history, lift magic number. v3

I reworked the approved patch to remove flushing from the test (and elsewhere, of course).

Now just includes the download limit and some tidying.

Let me know and I'll drop this into hg.

Comment 22

[Richard Newman [:rnewman]]

Landed history limit:

http://hg.mozilla.org/services/fx-sync/rev/095b98e852f8

Comment 23

[Richard Newman [:rnewman]]

Bug 617210 filed to bump download limit.