327330 - Speed up mork history import

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Brian Ryner (not reading)]

Currently when importing history entries, we call setPageDetails() and then addVisit() to add a visit on the last visit date.  This does more queries than we really need to since in both cases it checks whether the page already exists in history.  We could combine these into a single (non-interface) method on nsNavHistory and reduce the amount of querying.

Comment 1

[Brian Ryner (not reading)]

taking and resummarizing

Comment 2

[Brian Ryner (not reading)]

Attachment: many improvements

The changes here are somewhat extensive.  Here are the highlights:

nsMorkReader:
- Rather than keeping a per-row hashtable of column id to value, I'm now just keeping a nsTArray<nsCString> per row, with the array indexes corresponding to a global column array.  This reduces hashtable operations significantly.

- Atom map _keys_ (not values) now use a 9-byte string buffer built into the key, rather than a heap-allocated string (8 bytes for the string representation of a 32-bit key, plus a byte for the terminating null).  This reduces heap allocations significantly for parsing the atom map.  It does increase the amount of memcpy work when we grow the atom table, however, so one optimization we could do if we need to get more speed is to come up with a heuristic for initially sizing the atom table based on the size of the file.

- Don't MorkUnescape column ids which are atom keys (in practice that's all column ids)

- Consider a colon as a terminator for a row id.  Not doing this was causing us to treat table edits as new rows, probably causing duplicate entries and lost data.

nsNavHistory:
- Added new method AddPageWithVisit, which adds a new entry to history (without checking to see whether it's already there) and optionally adds a visit, doing everything with just 2 statement executions (and the statements cached).

nsMorkHistoryImporter:
- Use AddPageWithVisit, and update for nsMorkReader changes

nsStorageFormHistory:
- Update for nsMorkReader changes

Comment 3

[Brett Wilson]

Comment on attachment 213408 [details] [diff] [review]
many improvements

I'm a little uncomfortable with not checking URLs to see if they already exist. However, from my timing experiments we can only check 5000 URLs for existance per second on my powerbook. My history contains 10K items, so this isn't really fast enough. You should be sure the code can not call update for the same item more than once, because we'll get really confused if this happens. Also, we should be sure that this happens before bookmark or default_places import since those will both create history items.

Comment 4

[Brian Ryner (not reading)]

Attachment: better patch

implements the post-import duplicate pruning we talked about

Comment 5

[Brett Wilson]

Comment on attachment 213521 [details] [diff] [review]
better patch


>+nsNavHistory::InternalAddNewPage(nsIURI* aURI, const nsAString& aTitle,
>+                                 PRBool aHidden, PRBool aTyped,
>                                  PRInt32 aVisitCount, PRInt64* aPageID)
> {
>   mozStorageStatementScoper scoper(mDBAddNewPage);
>   nsresult rv = BindStatementURI(mDBAddNewPage, 0, aURI);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  // title
>+  rv = mDBAddNewPage->BindStringParameter(1, aTitle);

Check for isNull and use BindNullParameter(1) if so.

Comment 6

[Brian Ryner (not reading)]

checked in, trunk and branch

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looks like you forgot to check in the nsMorkReader.* changes, and it broke balsa.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, db/morkreader isn't in the tinderbox module...

Comment 9

[Nick Thomas [:nthomas] (UTC+12)]

biesi checked in a fix for balsa:

Index: mozilla/db/morkreader/nsMorkReader.cpp
===================================================================
RCS file: /cvsroot/mozilla/db/morkreader/nsMorkReader.cpp,v
retrieving revision 1.3
retrieving revision 1.4
diff -r1.3 -r1.4
564c564
<   nsAutoPtr< nsTArray<nsCString> > array = new nsTArray<nsCString>(aCount);
---
>   nsAutoPtr< nsTArray<nsCString> > array(new nsTArray<nsCString>(aCount));

Didn't land on the 1.8 branch yet.

(dbaron fixed the tinderbox module in bug 328979)

Comment 10

[Brett Wilson]

Nick's fix is now on the branch.

Comment 11

[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