first run migration / import from ie, opera and safari browser can be slow, bookmark import should use "run in batch" need to make the fix for bug #392003, but for bookmarks (bug #392003 was for history, which is typically much larger than bookmarks) nsIEProfileMigrator.cpp: rv = aBookmarksService->InsertBookmark(aParentFolder, resolvedURI, nsOperaProfileMigrator.cpp: rv = aBMS->InsertBookmark(onToolbar ? aToolbar : aParent, nsSafariProfileMigrator.cpp: rv |= aBookmarksService->InsertBookmark(aParentFolder, uri,

zoinks, it appears that we are using 0 (EXPIRE_SESSION) as the annotation expiration, instead of EXPIRE_NEVER

oops, wrong bug.

crashes every time at: http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistoryResult.cpp#3009 error in xcode is EXC_BAD_INSTRUCTION. well no kidding.

Not going to continue to block on this. If you disagree with this decision, please renominate with reasons why we can't ship with this in final

Is this still something that needs to be done? The wip patch is still useful?

(In reply to comment #4) > Not going to continue to block on this. If you disagree with this decision, > please renominate with reasons why we can't ship with this in final > I have just filed a bug 418643 about slow bookmark deletion and I'm about to investigate whether we do not have similar problem with bookmark import. I have seen that importing 100 bookmarks can lead to execution of more than 7000 queries. I will add more details soon.

Importing bookmarks into clean Bookmarks Menu folder using script (nsIPlacesImportExportService.importHTMLFromFile). Counting statements inside of tracefunc (mozStorageConnection.cpp): no import -> 80 statements 10 bookmarks without folder -> 387 statements 10 bookmarks inside folder -> 437 statements 37 bookmarks and 2 folders -> 1499 statements 37 bookmarks and 2 folders and 1 livemark (32 items) -> 2316 statements 5824 bookmarks in 430 folders -> 101982 statements It is better than I expected from what I saw before (caused most likely by importing items from Places Library where any mouse movement generates hundreds of queries and where more observers are registered - bug 417042). However, there are definitely more statements than necessary. Please see attached log for frequency of SQL statements. Obviously, it is suboptimal, that during import there is a special update of the added date and that the last modification time is updated 3 times and so on. Let me know, if I should spend some time on this and come with a suggestion.

(In reply to comment #7) > Let me know, if I > should spend some time on this and come with a suggestion. > yes please!

We still want this, but not going to block on it.

I have reduced the number of sql statements for big file without data favicons down to 50%. Turning off call to History()->UpdateFrecency(childID, isBookmark) in InsertBookmark reduced it by another 20%. Dietrich please comment if we can calculate frecency later then when importing. I want to try to reduce the number of queries by another 5-10%. There are some statements which i did not investigate. However, no query is performed twice or trice as before anymore (see wip patch which comes soon).

Isn't frecency done on idle too? Do we really need to calculate it while importing?

This wip patch reduces many statements that were executed multiple times. It will probably consume more memory and needs adjustments therefore. However, I would like to hear some comment on the included ideas before I try to prepare final version.

Comment on attachment 305037 [details] [diff] [review] Reducing repeated statements (wip) don't mind my drive by review >+ // Imported attributes - this could be wasting of memory? >+ nsAutoString mHref; >+ nsAutoString mFeedUrl; >+ nsAutoString mIcon; >+ nsAutoString mIconUri; >+ nsAutoString mLastCharset; >+ nsAutoString mKeyword; >+ nsAutoString mPostData; >+ nsAutoString mWebPanel; >+ nsAutoString mItemId; >+ nsAutoString mMicsumGenURI; >+ nsAutoString mGeneratedTitle; >+ nsAutoString mDateAdded; >+ nsAutoString mLastModified; auto strings should only be used on the stack - use in classes is frowned upon > // See RunInBatchMode, mLock _must_ be set when batching I thought I fixed that comment when I removed that lock - guess not... > nsresult > nsNavBookmarks::BeginUpdateBatch() > { > if (mBatchLevel++ == 0) { > mozIStorageConnection* conn = DBConn(); > PRBool transactionInProgress = PR_TRUE; // default to no transaction on err > conn->GetTransactionInProgress(&transactionInProgress); > mBatchHasTransaction = ! transactionInProgress; > if (mBatchHasTransaction) > conn->BeginTransaction(); >+ >+ NS_ENSURE_TRUE(mBatchLastUpdateTime.Init(128), NS_ERROR_OUT_OF_MEMORY); >+ NS_ENSURE_TRUE(mBatchIsLivemarkFolder.Init(128), NS_ERROR_OUT_OF_MEMORY); >+ NS_ENSURE_TRUE(mBatchItemCount.Init(128), NS_ERROR_OUT_OF_MEMORY); > > ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver, > OnBeginUpdateBatch()) > } > mozIStorageConnection *dbConn = DBConn(); > mozStorageTransaction transaction(dbConn, PR_FALSE); So, I realize you didn't do this, but this transaction code is identical to what is at the top of this function. The top code should probably just be canned - at least the parts that do this (I'm only bringing this up because you are touching this function) > nsresult > nsNavBookmarks::EndUpdateBatch() > { > if (--mBatchLevel == 0) { >+ >+ mozStorageStatementScoper scope(mDBSetItemLastModified); >+ { >+ mBatchLastUpdateTime.EnumerateRead( >+ ExecFolderTimeStatementCallback, >+ mDBSetItemLastModified.get()); >+ } >+ >+ mBatchLastUpdateTime.Clear(); >+ mBatchIsLivemarkFolder.Clear(); >+ mBatchItemCount.Clear(); >+ > if (mBatchHasTransaction) > DBConn()->CommitTransaction(); > mBatchHasTransaction = PR_FALSE; > ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver, > OnEndUpdateBatch()) > } > return NS_OK; > } So, I don't understand why you are doing this in these functions - could you please add some comments explaining what's going on and why in the code? Anyone looking at and learning this code (me!) won't have any idea what's going on.

(In reply to comment #10) > Turning off call to History()->UpdateFrecency(childID, isBookmark) > in InsertBookmark reduced it by another 20%. Dietrich please comment if we can > calculate frecency later then when importing. (In reply to comment #11) > Isn't frecency done on idle too? Do we really need to calculate it while > importing? > yes, i think we can turn off frecency calculation while importing bookmarks. we do calculate a chunk of frecency after database creation (so, for history only) and that should be enough to get us by until some idle processing occurs. these bookmarks will still be available in the location bar, just not boosted in rank.

Comment on attachment 305037 [details] [diff] [review] Reducing repeated statements (wip) removing review request - please address comment #13 and comment #14.

This patch reduces number of SQL statements executed during HTML import down to 22%. A test file with 5.824 bookmarks in 430 folders is now imported with less then 22.000 statements instead of original almost 102.000 statements.

Comment on attachment 308850 [details] [diff] [review] Reduce number of SQL statements executed during import This bug seems to be a cause of bug 422158.

Can you please unrot this when you have time? I'd like to do a tryserver build to test how it affects startup when using the test file referenced here: https://bugzilla.mozilla.org/show_bug.cgi?id=329736#c9

(In reply to comment #18) > Can you please unrot this when you have time? I'd like to do a tryserver build > to test how it affects startup when using the test file referenced here: > https://bugzilla.mozilla.org/show_bug.cgi?id=329736#c9 This was quite rotten by your date updating patch (bug 393498), I had to revert your changes, because I need public method which does not trigger notifications. I imported 1MB file and exported it and they were the same with exception of misplaced descriptions - I have submitted a bug 424143, because this problem is already present in the code.

builds here: https://build.mozilla.org/tryserver-builds/2008-04-02_09:15-dietrich@mozilla.com-bug392193-optimize-bookmarks-import/

Comment on attachment 310768 [details] [diff] [review] Optimize bookmark importing Can't really tell from the bug history who wrote this patch, but I'm not a good reviewer. Dietrich or Mano would be better choices.

Bumping priority a notch, but not going to block 3.1: after the bazillion upgrades we've had so far, there are few complaints of slow migration.

This is Ondrej's patch removing idl changes, it does not work actually since i still have to go through it and cleanup. i'm just trying to see if we can build up something we could take for 3.5. The first patch instead (dietrich's one) can help bug 490035, so i moved there a slightly modified version of it. These 2 patches are practically unrelated since Dietrich's one is about migration from another browser, while Ondrej's one is about importing from html. At this point the bugs should be splitted, just waiting to see where bug 490035 is going.

Comment on attachment 376133 [details] [diff] [review] unbitrot Ondrej's patch without idl changes - wip 0.1 Let's make some order, this bug will provide batching to migrators, and i'm going to file a new bug to reduce query load on importing from HTML.

moved import from HTML optimizations to bug 492384.

this is based on dietrich's first patch, adds batching when importing bookmarks, making import faster.

Comment on attachment 376739 [details] [diff] [review] patch v1.0 >+++ b/browser/components/migration/src/nsIEProfileMigrator.cpp >+NS_IMETHODIMP >+nsIEProfileMigrator::RunBatched(nsISupports* aUserData) >+{ >+ PRUint8 batchAction; >+ nsCOMPtr<nsISupportsPRUint8> strWrapper(do_QueryInterface(aUserData)); assert that strWrapper is not null please >+ nsCOMPtr<nsIProperties> fileLocator = >+ do_GetService("@mozilla.org/file/directory_service;1", &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ nsCOMPtr<nsIFile> favoritesDirectory; >+ fileLocator->Get("Favs", NS_GET_IID(nsIFile), >+ getter_AddRefs(favoritesDirectory)); cast return result to void since we don't care >+ aBMS->GetToolbarFolder(&folderId); cast result to void >+++ b/browser/components/migration/src/nsIEProfileMigrator.h Can you add java-doc style comments on these please so that it's a bit better documented >+++ b/browser/components/migration/src/nsOperaProfileMigrator.cpp >@@ -1076,8 +1138,10 @@ nsOperaProfileMigrator::CopyBookmarks(PR > sourceNameStrings, 1, > getter_Copies(importedOperaHotlistTitle)); > >- bms->CreateFolder(parentFolder, NS_ConvertUTF16toUTF8(importedOperaHotlistTitle), >- nsINavBookmarksService::DEFAULT_INDEX, &parentFolder); >+ bms->CreateFolder(parentFolder, >+ NS_ConvertUTF16toUTF8(importedOperaHotlistTitle), >+ nsINavBookmarksService::DEFAULT_INDEX, >+ &parentFolder); Check result, or cast to void please >@@ -1115,16 +1178,12 @@ nsOperaProfileMigrator::CopySmartKeyword > smartKeywords->Append(NS_LITERAL_STRING("search.ini")); > > nsCOMPtr<nsILocalFile> lf(do_QueryInterface(smartKeywords)); >- if (!lf) >- return NS_OK; >- > nsINIParser parser; >- rv = parser.Init(lf); >- if (NS_FAILED(rv)) >+ if (!lf || NS_FAILED(parser.Init(lf))) > return NS_OK; > > nsString sourceNameOpera; >- aBundle->GetStringFromName(NS_LITERAL_STRING("sourceNameOpera").get(), >+ aBundle->GetStringFromName(NS_LITERAL_STRING("sourceNameOpera").get(), > getter_Copies(sourceNameOpera)); ditto >@@ -1201,7 +1259,8 @@ nsOperaProfileMigrator::CopySmartKeyword > nsINavBookmarksService::DEFAULT_INDEX, > name, &newId); > NS_ENSURE_SUCCESS(rv, rv); >- // TODO -- set bookmark keyword to keyword and description to keywordDesc. >+ aBMS->SetKeywordForBookmark(newId, NS_ConvertUTF8toUTF16(keyword)); ditto (note: only change this where you are already touching code please) >+ // TODO -- set bookmark description to keywordDesc. Is there a bug filed on this? If it is, please add the bug number, otherwise file it and add the bug number please >+++ b/browser/components/migration/src/nsOperaProfileMigrator.h ditto on javadoc comments again >+++ b/browser/components/migration/src/nsSafariProfileMigrator.cpp >+NS_IMETHODIMP >+nsSafariProfileMigrator::RunBatched(nsISupports* aUserData) I feel like we need some base class that has all this shared code in it (don't worry about this for this bug) >@@ -900,7 +963,8 @@ nsSafariProfileMigrator::CopyBookmarks(P > sourceNameStrings, 1, > getter_Copies(importedSafariBookmarksTitle)); > >- bms->CreateFolder(root, NS_ConvertUTF16toUTF8(importedSafariBookmarksTitle), >+ bms->CreateFolder(bookmarksMenuFolderId, >+ NS_ConvertUTF16toUTF8(importedSafariBookmarksTitle), > nsINavBookmarksService::DEFAULT_INDEX, &folder); ditto on checking or cast to void >@@ -919,7 +983,8 @@ nsSafariProfileMigrator::CopyBookmarks(P > safariBookmarksFile->Append(NS_LITERAL_STRING("Safari")); > safariBookmarksFile->Append(SAFARI_BOOKMARKS_FILE_NAME); > >- CFDictionaryRef safariBookmarks = (CFDictionaryRef)CopyPListFromFile(safariBookmarksFile); >+ CFDictionaryRef safariBookmarks = >+ (CFDictionaryRef)CopyPListFromFile(safariBookmarksFile); use static_cast please instead of c-style cast >+++ b/browser/components/migration/src/nsSafariProfileMigrator.h ditto on javadoc comments again r=sdwilsh with these changes

addressed comments

Comment on attachment 376794 [details] [diff] [review] patch v1.1 Asking approval for this performance fix that will make migration faster.

Marco (or others) can you provide some risk assessment here? This is really late, and would ship without beta feedback. Also, how much faster are we talking about in practice? Reducing SQL statements is great, but I'd rather not risk this...

(In reply to comment #31) > Marco (or others) can you provide some risk assessment here? This is really > late, and would ship without beta feedback. Also, how much faster are we > talking about in practice? Reducing SQL statements is great, but I'd rather > not risk this... This patch does not really reduce the number of executed statements. It executes them all in a batch transaction, which can have huge performance benefits. It would be nice to get some ballpark numbers on this, so Marco should definitely post times to import the 10k bookmark file via these codepaths. The main part of this patch is taking the pre-existing code, and executing it in a batch callback. The executed code itself did not really change. Most of the changes in the patch in those sections of code are whitespace, indentation, etc for readability. So the net risk should be pretty low, since the actual code for the importing itself is not really changing.

Yeah - there really isn't a logic change here - just a re-factoring to make the import happen inside a transaction.

This also avoid us to try syncing temp tables continuously during the batch, that is completely useless. I'll try to get some number, but the "unresponsive script" dialog does not help since iterrupts me. Also i want to install all major browser and test bookmarks and history import, this is on my plans.

tested IE and Opera so far, moving on testing Safari. This fixed a wrong continue in opera smart keywords import (that was not working, but now works). I'll provide a roll-up patch for 1.9.1 with some number once i've finished testing.

Comment on attachment 377250 [details] [diff] [review] followup patch, fix Opera migrator r=sdwilsh

So, just to put some number here, i just tested importing an Internet Explorer large profile (about 10100 bookmarks): latest branch nightly: about 3 minutes latest trunk nightly: about 7 seconds Plus, as soon as i push the above Opera patch, i've tested all major migrators (IE, Opera, Safari), and they are working fine. As a bonus, now we import keywords from Opera (so this will fix also bug 464659).

If you show me a rollup and convince me there are tests, I will be happy to approve this patch.

there aren't test, migration component does not have ANY test. That's why i want to file a followup bug against it to add tests! And create those tests now would be a quite large project (each browser uses different files/formats/datastore), could take far more than a week. The changes are not big, we are adding batching, don't bother looking at the above comments since they refer to a previous approach that was changing a bunch of code to reduce query load. I tested this manually and Tracy confirmed me QA always executes migration tests on major platforms/browser before a release.

Yeah - this is something that is hard to test. I'd be OK with hand-run tests (which we have) for the time being.

pushed followup for Opera Migrator http://hg.mozilla.org/mozilla-central/rev/dd3b6f9f8e4a

This is the rollup patch for the above changesets.

Comment on attachment 377399 [details] [diff] [review] rollup patch for 1.9.1 a=mconnor, this should land ASAP.

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