614787 - Delay expiration database setup to the first expiration

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Creating the temp table is locking us on startup and is not even really useful till the first expiration runs. We can delay that creation to run with the first expiration, async too.

Comment 1

[Marco Bonardo [:mak]]

This is blocking Places branch merge, since it's part of our Ts regression.

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 3

[Marco Bonardo [:mak]]

note: expiration tests are already testing the existence of the table, thus this will be tested.

Comment 4

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> Creating the temp table is locking us on startup and is not even really useful
> till the first expiration runs. We can delay that creation to run with the
> first expiration, async too.
Do we have data showing an improvement in Ts if we disable this?

Comment 5

[Marco Bonardo [:mak]]

I have done a bunch of measures on try and the combination of this patch and the patch in Bug 614790 gave me the best results.

I can't give numbers for just this one since Bug 614790 covers any improvement. Btw this is work we absolutely don't need to do on startup and for a bunch of minutes after startup, so I can't really think to any reason to not do it.

Comment 6

[Marco Bonardo [:mak]]

Actually, we could get a better measure of the effect on branch, since this is useless work I think we would want this patch regardless and we won't need backouts or stuff like that.

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 493225 [details] [diff] [review]
patch v1.0

r=me. existing tests should be enough here, correct?

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 493225 [details] [diff] [review]
patch v1.0

>+    // The first expiration will also create the notifications temp table.
>+    let createTempTableStmt = this._createTempTableStmt;
>+    if (createTempTableStmt) {
>+      boundStatements.push(createTempTableStmt);
>+    }
When would this fail?

Comment 9

[Marco Bonardo [:mak]]

(In reply to comment #8)
> Comment on attachment 493225 [details] [diff] [review]
> patch v1.0
> 
> >+    // The first expiration will also create the notifications temp table.
> >+    let createTempTableStmt = this._createTempTableStmt;
> >+    if (createTempTableStmt) {
> >+      boundStatements.push(createTempTableStmt);
> >+    }
> When would this fail?

this doesn't fail, the getter is built so that only the first invocation returns something, next ones will return null, indeed only the first expiration has to create the temp table.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> this doesn't fail, the getter is built so that only the first invocation
> returns something, next ones will return null, indeed only the first expiration
> has to create the temp table.
Oh, right.  This indirection is a bit confusing.  Could we just create the temp table in our db getter like nsPlacesAutoComplete does?

Comment 11

[Marco Bonardo [:mak]]

(In reply to comment #10)
> Oh, right.  This indirection is a bit confusing.  Could we just create the temp
> table in our db getter like nsPlacesAutoComplete does?

we could, but since we already fire a bunch of write statements in a transaction, it was easier to just add one rather than adding more work on sqlite. If you think the hit is uninteresting I can do a separate executeAsync in the db getter.

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> we could, but since we already fire a bunch of write statements in a
> transaction, it was easier to just add one rather than adding more work on
> sqlite. If you think the hit is uninteresting I can do a separate executeAsync
> in the db getter.
It won't be in the transaction, but I don't think it really matters.  It should only be changing in-memory data structures for SQLite.  I think the code clarity will make it worthwhile though.

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v1.1

ok, based on comment 10. I'm not going to re-ask review, since it's mostly on the form rather than the content.

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 493753 [details] [diff] [review]
patch v1.1

r=sdwilsh

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/c952bf835f6b

Comment 16

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/c952bf835f6b