385066 - Remove preloading from mozStorage

Status: RESOLVED FIXED

(Toolkit :: Storage, defect, P3)

Description

[Shawn Wilsher :sdwilsh]

We had to disable it for Bug 341137, but it needs to be re-enabled, and preferably done upstream or in mozStorage code (not sqlite).

Comment 1

[Benjamin Smedberg]

Vlad, can you triage this? I don't know enough.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

So this was done originally to get back some startup performance; do we still have a startup perf issue?

Comment 3

[(not reading, please use [e-mail] instead)]

from brett's original work (bug #331158), he added this so that url bar
autocomplete would not be slow the first time.

I don't know if this blocks 1.9, but we care about it for firefox 3.

> do we still have a startup perf issue?

There might still be some "first time autocomplete performance issues", but I'll have to confirm that.

Comment 4

[Dietrich Ayala (:dietrich)]

Places has a 2% Ts penalty as of the end of June:

https://bugzilla.mozilla.org/show_bug.cgi?id=374532#c7

Comment 5

[Shawn Wilsher :sdwilsh]

Another issue to consider here is do we really want to patch our own sqlite?  It makes upgrading *a lot* harder to do (which is why it was disabled when we upgraded initially).

Comment 6

[(not reading, please use [e-mail] instead)]

to solve the "first time autocomplete performance issue", without patching sqlite and making upgrade harder, what about doing an autocomplete query that would (in effect) pre-flight the db cache?

in other words, get the "first time autocomplete performace issue" out of the way by doing an autocomplete (in delayed startup, so we don't affect Ts?) so that the first time a user does one, we've payed the price.

thoughts?

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> to solve the "first time autocomplete performance issue", without patching
> sqlite and making upgrade harder, what about doing an autocomplete query that
> would (in effect) pre-flight the db cache?
> 
> in other words, get the "first time autocomplete performace issue" out of the
> way by doing an autocomplete (in delayed startup, so we don't affect Ts?) so
> that the first time a user does one, we've payed the price.

This seems worth trying.

Comment 8

[Robert Sayre]

Not clear what the gain is here. If there's a Ts win, it would qualify for wanted-1.9.

Comment 9

[Brett Wilson]

(In reply to comment #6)
> ...
> thoughts?

This isn't nearly enough. The problem is that sqlite brings pages in on-demand and in many cases, in random order. This hits *very* hard on disk seeks.

Darin had a loptop that was particularly bad. Firefox would start up quickly, but the first time he pressed a key, it took 14 *seconds* for the results to come back. The preload patch had almost no impact on startup time, and made this result come back almost instantly.

You will still need to pay that 14 seconds, even if you don't do it on startup, as long as you bring in the pages by doing queries. Plus, you don't really know which queries to do to bring in the database, since most queries won't hit most of the pages.

I spent many weeks on this. I do not think there is another way.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #9)
> You will still need to pay that 14 seconds, even if you don't do it on startup,
> as long as you bring in the pages by doing queries. Plus, you don't really know
> which queries to do to bring in the database, since most queries won't hit most
> of the pages.

Would it be possible to construct a specific query that retrieves all relevant data from the history table and execute that query in a background thread on delayed startup so that all relevant pages get loaded into the cache within 14 seconds without delaying startup or hacking sqlite?

Comment 11

[Brett Wilson]

That will still give you 14 seconds of solid disk activity which is not OK, even on a background thread.

Comment 12

[Brett Wilson]

Plus you still wouldn't get autocomplete for that time, and people want to navigate after starting their browser.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> Would it be possible to construct a specific query that retrieves all relevant
> data from the history table and execute that query in a background thread on
> delayed startup so that all relevant pages get loaded into the cache within 14
> seconds without delaying startup or hacking sqlite?
Also, cache's are thread specific, so if you run it on a background thread, it's completely useless.

Brett - is there any reason this change wasn't pushed back up to sqlite?  The reason why this wasn't kept when we upgraded was because the patch didn't apply cleanly, and thunder and I didn't understand what was going on well enough to try and manually patch it.

As it stands, I'm rather against patching sqlite in only our tree now, but if we can preload without touching sqlite3.c, I'm all for it.

Comment 14

[Robert Sayre]

(In reply to comment #9)
> 
> I spent many weeks on this. I do not think there is another way.

Hey Brett, thanks for the background information. It's really helpful.

Comment 15

[Brett Wilson]

I brought this up with Richard Hipp, the sqlite author, but it didn't really go anywhere (not that he was necessarily against it). Maybe you guys should submit it to him.

Assuming he doesn't take it, why wouldn't you patch it? It takes < 30 minutes to fix the patch to match the renamed functions and moved code that happened in the last year.

Comment 16

[Brett Wilson]

Attachment: Updated sqlite diff

This is an updated patch for sqlite I put together last night after the discussion to see if it was as easy as I said. It took me about 40 minutes because I found a bug in the old version! When I loaded a page, the old version would always load page 1, whereas it was supposed to load all the pages. The old patch sped up the system just by priming the OS cache, not sqlite's.

This patch is for sqlite 3.4.2 because that's what I checked out, it looks like Mozilla has 3.4.1.

This isn't a patch for Mozilla, as I don't have a very recent checkout, I did this on a plain sqlite checkout. Maybe somebody can make a patch for Mozilla that checks this in and applies it.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #15)
> I brought this up with Richard Hipp, the sqlite author, but it didn't really go
> anywhere (not that he was necessarily against it). Maybe you guys should submit
> it to him.
> 
> Assuming he doesn't take it, why wouldn't you patch it?

Given that there don't seem to be better options, this indeed seems like something we should patch locally and try again to push up.

Comment 18

[Brett Wilson]

Actually, since my old patch was basically bogus and only primed the OS cache. I wonder if this would be sufficient? It would not require modifications to sqlite. I think there will have to be some benchmarks done to decide what to do here.

Comment 19

[Dietrich Ayala (:dietrich)]

Attachment: mozified + enabled

here's brett's preload change, patched into the moz amalgamated sqlite source.

i tested this patch on a profile w/ a large places.sqlite (50+mb), and startup hangs for about 5 seconds while preloading.

Comment 20

[Shawn Wilsher :sdwilsh]

Re-requesting blocking.  We need one of the two things decided here:
1) enable preloading (which appears to have a Ts perf impact)
2) remove preloading from our API

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We should remove preloading; there's no need to patch our sqlite any more, and the significant Ts impact makes this not worth it.  We should file another bug to resolve the first-time autocomplete hit, but that's a separate thing (in any case, such a thing should be done in a timer, and not at app startup).

Comment 22

[Shawn Wilsher :sdwilsh]

Attachment: remove v1.0

Comment 23

[Adam Kowalczyk]

Why I we removing this capability altogether instead of just disabling it for Places? Perhaps other consumers, including extensions, could still find it useful.

Comment 24

[Shawn Wilsher :sdwilsh]

Because I'm not inclined to patch sqlite just for addons.

Comment 25

[Reed Loden [:reed]]

Checking in toolkit/components/satchel/src/nsStorageFormHistory.cpp;
/cvsroot/mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp,v  <--  nsStorageFormHistory.cpp
new revision: 1.20; previous revision: 1.19
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.214; previous revision: 1.213
done
Checking in db/sqlite3/README.MOZILLA;
/cvsroot/mozilla/db/sqlite3/README.MOZILLA,v  <--  README.MOZILLA
new revision: 1.16; previous revision: 1.15
done
Removing db/sqlite3/preload-cache.patch;
/cvsroot/mozilla/db/sqlite3/preload-cache.patch,v  <--  preload-cache.patch
new revision: delete; previous revision: 1.2
done
Checking in storage/public/mozIStorageConnection.idl;
/cvsroot/mozilla/storage/public/mozIStorageConnection.idl,v  <--  mozIStorageConnection.idl
new revision: 1.14; previous revision: 1.13
done
Checking in storage/src/mozStorageConnection.cpp;
/cvsroot/mozilla/storage/src/mozStorageConnection.cpp,v  <--  mozStorageConnection.cpp
new revision: 1.30; previous revision: 1.29
done

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Also, it's questionable at best whether this really helps in today's sqlite world -- it was originally done to fix a specific performance issue that's no longer present to anywhere the same degree.

Comment 27

[Eric Shepherd [:sheppy]]

Why is this marked as requiring a doc update?  This appears to be an internal functionality change rather than something that impacts docs.  What am I missing?

Comment 28

[Eric Shepherd [:sheppy]]

Never mind... didn't scroll down quite far enough.  I've marked the preload() function as obsolete and no longer available in Firefox 3 in the documentation for mozIStorageConnection.