Closed Bug 401753 Opened 17 years ago Closed 17 years ago

creating the "Places" folder (see bug #387996) for an existing profile with lots of bookmarks takes too long, the personal toolbar is blank for several seconds.

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta2

People

(Reporter: moco, Assigned: asaf)

References

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

creating the "Places" folder (see bug #387996) for an existing profile with lots of bookmarks takes too long, the personal toolbar is blank for several seconds.

this was with "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102923 Minefield/3.0a9pre"
Blocks: 399984
We should fix this by creating the places folder _before_ importing bookmarks.html, otherwise AdjustIndicies executes a pretty heavy UPDATE statement.
Assignee: nobody → mano
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Target Milestone: --- → Firefox 3 M11
Attached patch patch (obsolete) — Splinter Review
Attachment #290280 - Flags: review?
Attachment #290280 - Flags: review? → review?(sspitzer)
1)

+    // Call it here for Fx3 profiles created brefore the Places folder
+    // has been added, otherwise it's called during import.
+//    this.ensurePlacesDefaultQueriesInitialized();
+

shouldn't the last line be uncommented?

nit:  s/brefore/before

2)

+    // XXXmano: this should be batched even if we're not called from the
import
+    // service. However, calling runInBatchedMode from whithin a RunBatched
+    // implementation hangs the browser.

can you add a reference back to bug #405497?

[Bug 405497] New:   Calling RunInBatchedMode within a RunBatched implementation
hangs"

nit:  s/whithin/within 

3)

What about making ensurePlacesDefaultQueriesInitialized take a boolean for
whether or not to batch?
(In reply to comment #3)
> 1)
> 
> +    // Call it here for Fx3 profiles created brefore the Places folder
> +    // has been added, otherwise it's called during import.
> +//    this.ensurePlacesDefaultQueriesInitialized();
> +

> shouldn't the last line be uncommented?

> 
yeah, oops
> +    // XXXmano: this should be batched even if we're not called from the
> import
> +    // service. However, calling runInBatchedMode from whithin a RunBatched
> +    // implementation hangs the browser.
> 
> can you add a reference back to bug #405497?
>
done
 
> 
> What about making ensurePlacesDefaultQueriesInitialized take a boolean for
> whether or not to batch?

I don't it's worth bloating the api for this kinda edge case (which I'm hoping to fix right before final).
Comment on attachment 290280 [details] [diff] [review]
patch

clearing review request.

mano, can you attach an updated version?
Attachment #290280 - Flags: review?(sspitzer)
Attached patch patch (obsolete) — Splinter Review
Attachment #290280 - Attachment is obsolete: true
Attachment #290288 - Flags: review?
Attachment #290288 - Flags: review? → review?(sspitzer)
Comment on attachment 290288 [details] [diff] [review]
patch

r=sspitzer, with one concern.

over irc you wrote:

<Mano> fyi, my testing shows that we don't need to call savePrefFile once i moved the call to setBoolPref to browserglue
<sspitzerMsgMe> is that because something else is calling savePrefFile()?
<Mano> very likely

can you set a breakpoint and figure out who is calling savePrefFile() in both the existing (but no "places" folder) and the new profile scenario?

I just want to confirm that removing the call savePrefFile() is correct.
Attachment #290288 - Flags: review?(sspitzer) → review+
Seth, thanks for asking me to double check this, savePrefFile isn't called apparently, so I've added it back.
Attached patch for checkinSplinter Review
Attachment #290288 - Attachment is obsolete: true
mozilla/browser/base/content/browser.js 1.903
mozilla/browser/components/nsBrowserGlue.js 1.38
mozilla/browser/components/nsIBrowserGlue.idl 1.4
mozilla/browser/components/places/src/Makefile.in 1.25
mozilla/browser/components/places/src/nsPlacesImportExportService.cpp 1.36
mozilla/browser/components/places/tests/unit/test_bookmarks_html.js 1.14
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3 M11 → Firefox 3 M10
Comment on attachment 290300 [details] [diff] [review]
for checkin

r=sspitzer, thanks for double checking savePrefFile()
Attachment #290300 - Flags: review+
12ms Ts drop on bl-bldlnx03
9ms Ts drop on bm-xserve08
verified with monster bookmark profile on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007112804 Minefield/3.0b2pre
Status: RESOLVED → VERIFIED
this regressed the case when i clear my profile dir content and start minefiled, the places folder is not created.

works in 26/11, don't work in 27/11 (win) probably due to this checkin

1. delete the profile dir contents (not the dir)
2. launch minefield
3. places folder not created

i had not filled a new report since i don't know if it's expected behaviour, i often clear profile contents to test builds with a new clean profile. Should i fill a report on this?
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.