560309 - audit mozStorage usage in Weave

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Comment 1

[Mike Connor [:mconnor]]

http://mxr.mozilla.org/labs-central/search?string=statement&find=%2Fweave%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=labs-central is probably a good start

Comment 2

[Shawn Wilsher :sdwilsh]

First up: history.js

OK, so selecting from *_view is slow.  Very slow.  It doesn't use an indexes, so don't do it.  Ever.  You'll have to do the joins manually, but I bet mak can help there.

You are using step on mozIStorageStatement, which is deprecated.  It'll be removed probably after 1.9.3.  But really, you should be using executeAsync for all of this.  If you were using the places API, I'd deal with the sync I/O, but you are already rolling your own statements.

I bet these comments apply to all the files.

Comment 3

[Shawn Wilsher :sdwilsh]

forms.js

The query you have in getAllEntries is going to be nothing short of very slow since you have a bunch of subselect statements.

Are you really blindly adding the guid column to the form manager table when a statement fails to compile?  There are several better ways to do that (including getting it added in core).

Comment 4

[Shawn Wilsher :sdwilsh]

No comments on bookmarks.js, but you should file a bug to have someone on the places team (mak?) look over your API usage.

Comment 5

[Shawn Wilsher :sdwilsh]

fx-setup.js

Note that places doesn't keep track of X days of history anymore, so having weave sync Y days of history seems a bit odd.

Comment 6

[Shawn Wilsher :sdwilsh]

Audit done.  Someone needs to file bugs based on my feedback.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #3)
> Are you really blindly adding the guid column to the form manager table when a
> statement fails to compile?  There are several better ways to do that
> (including getting it added in core).

It has been added to core, but only very recently (bug 506402). Dolske was consulted on what Weave was doing before it happened. Though you're right that it could probably have a more explicit check for the column instead of just adding the it on failure.