Closed Bug 381750 Opened 18 years ago Closed 17 years ago

with lots of bookmarks, opening up the bookmark manager dialog is slow

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3 alpha5

People

(Reporter: moco, Assigned: moco)

References

Details

(Keywords: perf)

Attachments

(1 file)

patch
(deleted), patch
dietrich
: review+
Details | Diff | Splinter Review
with lots of bookmarks, opening up the bookmark manager dialog is slow once again, I'm using a profile built from bookmarks.html / history.dat from jay, and opening up the bookmark manager dialog is slow, on the order of 5 seconds. this is on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070522 Minefield/3.0a5pre that's on my debug build, but this is still a noticeable perf regression.
when we open the bm organizer, we attempt to set the selected node in the tree to be ORGANIZER_ROOT_BOOKMARKS the recursive findNode() helper function in in selectPlaceURI() will try to find and open the container that has that a child with that uri. the problem is ORGANIZER_ROOT_BOOKMARKS is the root, so we walk the whole tree and fail to our child because the container we pass in to findNode() is actually what we are searching for. about the performance problem, while walking we open and close containers. every time we open (or close) a container, treeView's containerOpened() gets called, which will call invalidateContainer(), which will call _refreshVisibleSection() which in turn will call _buildVisibleSection(). All of this is very expensive, especially when we do it for every container, recursively.
Assignee: nobody → sspitzer
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Firefox 3 alpha5
Attached patch patch (deleted) — — Splinter Review
Attachment #266319 - Flags: review?(dietrich)
Comment on attachment 266319 [details] [diff] [review] patch >Index: browser/components/places/content/tree.xml >=================================================================== >RCS file: /cvsroot/mozilla/browser/components/places/content/tree.xml,v >retrieving revision 1.70 >diff -u -8 -p -r1.70 tree.xml >--- browser/components/places/content/tree.xml 25 May 2007 23:50:21 -0000 1.70 >+++ browser/components/places/content/tree.xml 28 May 2007 02:28:22 -0000 >@@ -165,16 +169,19 @@ > Causes a particular node represented by the specified placeURI to be > selected in the tree. All containers above the node in the hierarchy > will be opened, so that the node is visible. > --> > <method name="selectPlaceURI"> > <parameter name="placeURI"/> > <body><![CDATA[ > function findNode(container, placeURI) { >+ if (container.uri == placeURI) >+ return container; >+ nit: this seems kind of awkward because the context in which findNode is called assumes searching for children of |container|. maybe should this check be outside of findNode? either way, r=me. > container.containerOpen = true; > for (var i = 0; i < container.childCount; ++i) { > var child = container.getChild(i); > if (child.uri == placeURI) > return child; > else if (PlacesUtils.nodeIsContainer(child)) { > var nested = findNode(asContainer(child), placeURI); > if (nested) >Index: browser/components/places/content/controller.js >=================================================================== >RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v >retrieving revision 1.154 >diff -u -8 -p -r1.154 controller.js >--- browser/components/places/content/controller.js 25 May 2007 23:50:21 -0000 1.154 >+++ browser/components/places/content/controller.js 28 May 2007 02:28:22 -0000 >@@ -36,17 +36,17 @@ > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > const NHRVO = Ci.nsINavHistoryResultViewObserver; > > // XXXmano: we should move most/all of these constants to PlacesUtils > const ORGANIZER_ROOT_HISTORY_UNSORTED = "place:beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&type=1"; >-const ORGANIZER_ROOT_BOOKMARKS = "place:folder=2&group=3&excludeItems=1"; >+const ORGANIZER_ROOT_BOOKMARKS = "place:folder=2&group=3&excludeItems=1&queryType=1"; > const ORGANIZER_SUBSCRIPTIONS_QUERY = "place:annotation=livemark%2FfeedURI"; > > // No change to the view, preserve current selection > const RELOAD_ACTION_NOTHING = 0; > // Inserting items new to the view, select the inserted rows > const RELOAD_ACTION_INSERT = 1; > // Removing items from the view, select the first item after the last selected > const RELOAD_ACTION_REMOVE = 2; was this intentionally included in this patch? setting a folder does make queryType unnecessary, so the change is ok, but checking since it wasn't mentioned in the bug comments.
Attachment #266319 - Flags: review?(dietrich) → review+
> was this intentionally included in this patch? yes, sorry, it was intentional/ Later in findNode() we look for uris to match, and our container nodes have queryType=1 appended when we get their uri (ex: child.uri).
Status: NEW → ASSIGNED
fixed. Checking in browser/components/places/content/controller.js; /cvsroot/mozilla/browser/components/places/content/controller.js,v <-- control ler.js new revision: 1.155; previous revision: 1.154 done Checking in browser/components/places/content/tree.xml; /cvsroot/mozilla/browser/components/places/content/tree.xml,v <-- tree.xml new revision: 1.71; previous revision: 1.70 done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: perf
Resolution: --- → FIXED
Comment on attachment 266319 [details] [diff] [review] patch trailing spaces suck too ;)
> trailing spaces suck too ;) oops, sorry about that. In bug #382195 (pending review) I have removed the trailing white space.
verified with Windows trunk build from 20070620
Status: RESOLVED → VERIFIED
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.

Attachment

General

Creator:
Created:
Updated:
Size: