488571 - Get rid of explicit form history caching

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

The form history code currently does some odd things for caching... It creates a dummy table with a single dummy row, then creates a dummy statement that accesses the table, but leaves the statement open. The comments indicate this is supposed to force the normal moz_formhistory table to stay permanently cached in memory. Oh, and it also sets the cache size to 4MB, which seems rather excessive.

I talked with sdwilsh about this, and he's not sure why it's doing this either. Other DB code in the tree doesn't seem to do anything like this. SQLite should already be doing it's own caching transparently.

This ends up being a problem for bug 243136, because we can't VACUUM the DB (to shrink the file size after expiring lots of entries), because the open statement blocks the vacuum statement from executing (it gets a SQLITE_BUSY error).

I'll gather some perf numbers to confirm this shouldn't be a problem.

Comment 1

[Justin Dolske [:Dolske]]

I ran some tests with the query used by form history to populate the autocomplete dropdown, since that's the post performance sensitive thing. I used a formhistory.sqlite from my normal daily profile -- 2.5MB, 27,315 entries.

The times were identical to within a few ms, comparing a current trunk nightly with my trunk build plus this patch.

The timed query was:

SELECT value FROM moz_formhistory WHERE fieldName='q' ORDER BY UPPER(value) ASC

For fieldName 'q'           (12,605 entries), it took 190ms.
For fieldName 'blocked'     (   451 entries), it took   6ms.
For fieldName 'assigned_to' (   200 entries), it took   3ms.
For fieldName 'wpSummary'   (   135 entries), it took   3ms.
For fieldName 'qt'          (    21 entries), it took   1ms.

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 372974 [details] [diff] [review]
Patch v.1

r=sdwilsh

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Bug 332935 suggests this was added to avoid problems in cases where I/O is slow (e.g. networked drives). Are we sure that this won't cause a performance regression in those cases?

Comment 4

[Justin Dolske [:Dolske]]

I'm not sure, but that seems like a general issue that should be addressed in SQLite itself. I don't know of any other DB code in the tree that does this, and sdwilsh says he's not aware of bugs being filed on the issue. That would lead me to hope that something has changed in SQLite in the past 3 years to improve the situation that bug 332935 was originally addressing.

Comment 5

[Brett Wilson]

Previous versions of sqlite would discard the pager cache between commits which
was very detrimental to performance. Since this data structure is used for link
coloring, basically the entire database is needed to render any web page, and
losing this cache is very bad.

This has been fixed in sqlite. I don't know which revision, but I think
anything in the last year or so should be fine.

Because of the need to use large amounts of the database, you really want it to
be in memory which is the explanation for the large cache size. The OS cache
will help some, but that gets discarded more often, and performance drops
rapidly for even a bit more latency added here since there can be 200 visited
link queries for a complex page like yahoo.com.

This space isn't used if your database isn't that large, so there isn't a
penalty for users with smaller databases. I would not recommend decreasing the
cache size. Chromium's cache size is currently set to 24MB (most of which is
usually not needed).

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Are you confusing this code with the Places history code? Form autocomplete data shouldn't be required for link coloring...

Comment 7

[Brett Wilson]

Yes :)

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 372974 [details] [diff] [review]
Patch v.1

OK, I think getting rid of the dummy statement stuff is fine then. If we don't need this for other more performance-critical DBs, it shouldn't be necessary here, and it seems likely that this code was originally intended to work around an SQLite bug that's long since been fixed.

The cache size change (4000 to the default of 2000) is less obviously correct, but anecdotal evidence suggests that large form history databases are on the order of ~2.5MB, so the default 2MB cache should be fine, especially considering the expiry we're going to be getting with bug 243136.

Comment 9

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/52cf8394b2e0

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 372974 [details] [diff] [review]
Patch v.1

a191=beltzner

Comment 11

[Justin Dolske [:Dolske]]

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f154808b6cb6