483096 - Backport formhistory timestamps to FF3.0.x

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Justin Dolske [:Dolske]]

We're looking at landing smarter form autocomplete logic (ala awesomebar) for Firefox 3.5, but have found some lurking performance issues... Form history never expires its contents (bug 243136), which results in poor autocomplete responsiveness when there are gobs of results. For example, my daily-use profile has ~12,000 Google searches in it. 90% of those are more than 4 months old.

The current form autocomplete performance isn't too bad (a few hundred milliseconds to generate a menu for Google on my hardware), but this will get worse as users collect more form history. It also means that more complex queries (such as we'd like for Firefox 3.5) carry a disproportionate performance penalty, because they have to wade through a mountain of old data.

Trunk (and 1.9.1) is close to being able to deal with this -- bug 463154 added timestamps to form data, and bug 243136 will use the timestamps to expire sufficiently old entries.

The catch is that when we upgrade a profile's formhistory.sqlite, we set the timestamps of existing entries to the current time (since we don't have any idea when they were actually created). This means that if we want to expire entries older than (say) 90 days, we can't do so until 90 days after the upgrade point. Thus, if we added more complex queries to Firefox 3.5, users could experience poor performance for the first 90 days until we expire the pile of stale data.

The solution I think we should pursue is to immediately patch Firefox 3.0.x to add timestamps. Then user data will actually start aging, and by the time Firefox 3.5 is released there will be data eligible for expiration.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

First pass.

This backports the current trunk form storage backend. Primarily to pick up bug 463154 and bug 472064, and also two other small fixes (bug 426991, bug 468543). The API change from 463154 is not included. Put another way, this is basically trunk minus the private browsing changes and bug 464947.

This code has been baking on trunk (and 1.9.1) for a while -- 463154 landed in November, and 472064 landed in January. Those are the biggest changes, and have good test coverage.

[Note that a minimized part of bug 472064 landed for 1.9.0.7, this bug lands the whole thing.]

Comment 2

[Daniel Veditz [:dveditz]]

Not "blocking" but looks reasonable. Shouldn't we get a review on this collection of previous patches? At the very least a double-check that this includes all the pieces you think it does?

We'd strongly consider approving a reviewed patch, but the 1.9.0.8 code-freeze is tomorrow. Shipping in 1.9.0.9 probably wouldn't do you much good since that'll be pretty close to the Shiretoko ship.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Same as last patch, but added the mochitest test from 463486 for better test coverage.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Diff between trunk and patch v.2

This might (or might not) be useful, it's a diff generated by copying all the stuff from the branch patch onto a trunk tree.

Comment 5

[Justin Dolske [:Dolske]]

Requesting approval for 1.9.0.8... Overall risk is fairly low with this patch, it has already been baking on trunk and 1.9.1 (see comment 2). No interface changes.

The only potential area of risk I can think of is that if an existing extension issues raw SQL against formhistory.sqlite, it will most likely continue work but it's possible for it to get tripped up by the extra columns / schema version. I don't know of any extensions that do this, and it seems unlikely. Form history in Firefox 3.0-3.0.7 should continue to work even if the profile has been used in a 3.0.8+ / 3.5+ build with that updated the DB.

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 367663 [details] [diff] [review]
Patch v.2

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 7

[Justin Dolske [:Dolske]]

Checked in:

Checking in toolkit/components/satchel/src/nsStorageFormHistory.cpp;
  new revision: 1.24; previous revision: 1.23
Checking in toolkit/components/satchel/src/nsStorageFormHistory.h;
  new revision: 1.9; previous revision: 1.8
Checking in toolkit/components/satchel/test/Makefile.in;
  new revision: 1.2; previous revision: 1.1
Checking in toolkit/components/satchel/test/satchel_common.js;
  initial revision: 1.1
Checking in toolkit/components/satchel/test/test_form_submission.html;
  initial revision: 1.1
Checking in toolkit/components/satchel/test/unit/formhistory_CORRUPT.sqlite;
  initial revision: 1.1
Checking in toolkit/components/satchel/test/unit/formhistory_apitest.sqlite;
  new revision: 1.2; previous revision: 1.1
Checking in toolkit/components/satchel/test/unit/formhistory_v0.sqlite;
  initial revision: 1.1
Checking in toolkit/components/satchel/test/unit/formhistory_v0v1.sqlite;
  initial revision: 1.1
Checking in toolkit/components/satchel/test/unit/formhistory_v999b.sqlite;
  initial revision: 1.1
Checking in toolkit/components/satchel/test/unit/head_satchel.js;
  new revision: 1.3; previous revision: 1.2
Checking in toolkit/components/satchel/test/unit/test_db_corrupt.js;
  initial revision: 1.1
Checking in toolkit/components/satchel/test/unit/test_db_update_v1.js;
  initial revision: 1.1
Checking in toolkit/components/satchel/test/unit/test_db_update_v1b.js;
  initial revision: 1.1
Checking in toolkit/components/satchel/test/unit/test_db_update_v999b.js;
  initial revision: 1.1

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.8 using checked in tests, which are passing.