389876 - after places schema change to remove moz_places.user_title, first time start up is very slow

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

after places schema change to allow for expiration of annotations, first time start up is very slow

spun off of bug #319455 comments #47, #49, #53.

"It took nearly 5 minutes to appear (about 16 MB file)"
"18 minutes later and now my history has been processed"

I have a large pre-bug-319455 places.sqlite file and MigrateV6Up() takes me 25 seconds with my debug, windows build (but on a decently fast laptop)

stevee / ria:  do you still have the places.sqlite files that are taking you minutes?

Comment 1

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

maybe there is something we can do for m7.  taking a look...

Comment 2

[Steve England [:stevee]]

I only have the places.sqlite that has been processed by bug 319455, sorry; unfortunately I don't keep a 5-day backup of this file.

Comment 3

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

my numbers are all over the place on this one, nothing in hand yet to make it faster for m7.

Comment 4

[Ria Klaassen (not reading all bugmail)]

Mine is too privacy sensitive, and the older file that I once made for testing purposes doesn't show the problem. 

Comment 5

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #4)
> and the older file that I once made for testing
> purposes doesn't show the problem. 
>
No, not at all. When I run an old history.dat file several times in a pre Bug 319455 build the startup time is 8 seconds and in a post Bug 319455 build also 8 seconds.. 

Comment 6

[Ria Klaassen (not reading all bugmail)]

Well, that's not surprising if you have set your history to 9 days.. :/ 
Retested with 1000 days and the first startup takes 12 seconds for a places.sqlite file of about 3,5 MB. It only contains history of about 1 year ago and for some reason it doesn't show the problem.

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: wip patch

moves the column-removal to shutdown.

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

re-order expiration and shutdown-migration. thanks for testing that, seth.

Comment 9

[Shawn Wilsher :sdwilsh]

Is shutdown any better?  It seems like that could lead to more issues such as the user not being able to access their profile if they restart the browser...

Comment 10

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

> Is shutdown any better? 

yes, for two reasons:

1)  it's faster.  I found 26 - 28 seconds for on startup, vs 1.3 on startup + 12 on shutdown.

2)  we have ui on "slow shutdown" (see https://bugzilla.mozilla.org/attachment.cgi?id=270798).

3)  opinion: I think a one time, slow startup is more noticable and worse than a one time slow shutdown.

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

- don't unnecessarily create the title index
- do remove/create all moz_places indices
- comment about why we remove/create and when

Comment 12

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

Comment on attachment 274558 [details] [diff] [review]
fix v2

r=sspitzer, thanks for adding all those comments.

Comment 13

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.147; previous revision: 1.146
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.89; previous revision: 1.88
done

Comment 14

[Robert Strong (they/them - no direct email)]

Is the OS shutdown case going to be an issue here?

Comment 15

[Dietrich Ayala (:dietrich)]

Seth's 50+mb places.sqlite did the table copy in 12 seconds on shutdown. I don't know how much time windows gives in the event of an automated shutdown, and there'll always be a user with 100mb file, or whatever. For those cases, I'd have to fall back to Dr. Hipp: http://www.mail-archive.com/[e-mail]/msg23702.html. Basically, SQLite wraps every query that changes the database in a transaction. If there's an uncontrolled shutdown, the transaction is rolled back the next time the db is opened (except for the exceptions in section 6 of the above referenced link). Since this code is run on shutdown, it'll check again the next time a shutdown occurs.

Also note that this is a concern with every single consumer of SQLite in our tree, for the entire duration of the application's lifetime. FWIW, I've hard-stopped our nightlies many times over, and have not seen any corruption to date.

However, one change we should make is to wrap the entire contents of CleanUpOnQuit(). Patch forthcoming.

Comment 16

[Dietrich Ayala (:dietrich)]

Attachment: followup

followup patch per comment 15.

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 274571 [details] [diff] [review]
followup

r+a=me

Comment 18

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.149; previous revision: 1.148
done

Comment 19

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

note to dietrich:

adding that transaction made things faster (if you believe my numbers from my debug build, which we've been believing so far).

expire is still about 1 second, but cleanup on quit is now 9 seconds (down from 12).  export to html of bookmarks is still .35 seconds.

nice!

I'm interested to hear from Steve and Ria if they have some numbers comparing start up / shut down before this patch vs start up / shut down after (with the same "old" places.sqlite file).

to summarize my numbers with ispiked's places.sqlite:

start up:  migratev6up + droping user_title, ~26 seconds
shut down: expire + export to html, ~2 seconds, not sure?

vs

start up:  migratev6up, ~1 second
shut down: cleanup on quit + expire + export to html, ~10 seconds

Comment 20

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

Now that this is resolved, do we have a good places.sqlite that can repro this issue pre-fix?

How do we verify that the fix is really in (pardon the pun)? :-)

Comment 21

[Dietrich Ayala (:dietrich)]

verification steps:

1. with a pre-patch profile, start a build that contains the fix

2. open the places.sqlite in sqlite db browser, confirm that moz_places.user_title is there, close db browser

3. close the browser

4. open the places.sqlite in sqlite db browser, confirm that moz_places.user_title is not there

Comment 22

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007080204 Minefield/3.0a7pre

Comment 23

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv