Closed Bug 614787 Opened 9 years ago Closed 9 years ago

Delay expiration database setup to the first expiration

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mak, Assigned: mak)

Details

(Whiteboard: [fixed-in-places])

Attachments

(1 file, 1 obsolete file)

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.
This is blocking Places branch merge, since it's part of our Ts regression.
blocking2.0: --- → ?
Flags: in-testsuite?
Attached patch patch v1.0 (obsolete) — Splinter Review
Attachment #493225 - Flags: review?(dietrich)
note: expiration tests are already testing the existence of the table, thus this will be tested.
(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?
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.
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 on attachment 493225 [details] [diff] [review]
patch v1.0

r=me. existing tests should be enough here, correct?
Attachment #493225 - Flags: review?(dietrich) → review+
blocking2.0: ? → betaN+
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?
(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.
(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?
(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.
(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.
Attached patch patch v1.1Splinter Review
ok, based on comment 10. I'm not going to re-ask review, since it's mostly on the form rather than the content.
Attachment #493225 - Attachment is obsolete: true
Comment on attachment 493753 [details] [diff] [review]
patch v1.1

r=sdwilsh
Attachment #493753 - Flags: review+
http://hg.mozilla.org/projects/places/rev/c952bf835f6b
Flags: in-testsuite? → in-testsuite+
Whiteboard: [fixed-in-places]
http://hg.mozilla.org/mozilla-central/rev/c952bf835f6b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b9
You need to log in before you can comment on or make changes to this bug.