Closed Bug 490664 Opened 16 years ago Closed 11 years ago

double clicking on a folder in the right pane opens really slow

Categories

(Firefox :: Bookmarks & History, defect)

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

Tracking

()

Status:
RESOLVED INCOMPLETE
Tracking Flags:
Tracking Status
status1.9.1 --- ?

People

(Reporter: dietrich, Unassigned)

References

Details

(Keywords: perf, Whiteboard: [TSnappiness][needs new patch])

Attachments

(1 file, 4 obsolete files)

unrotted
9.88 KB, patch
Details | Diff | Splinter Review
hypothesis is that the left-pane container-opening code is very inefficient, opening closed containers. possibly is a cause of bug 489038.
Whiteboard: [TSnappiness]
performance is even worse when doing this in the library: 1. dbl click on right-pane folder 2. click the back button long beachball when hitting back.
Assignee: nobody → dietrich
Attached patch wip (obsolete) — — Splinter Review
low-bar patch to only search through already open containers when opening bookmark folders in the right pane. doesn't address any other kind of container, nor the back button problem.
Attached patch more (obsolete) — — Splinter Review
this changes selection of bookmark folders and all other container nodes so that when selecting a node/uri in the tree, visible containers are searched first, and then non-visible containers. while this speeds up navigating around the library in some ways, opening up tag containers is still not fast for me :(
Attachment #375424 - Attachment is obsolete: true
some testing notes, since i'm not sure about how to reasonably automate this: for history date containers, tag containers and bookmark folders, do the following: steps: in the left pane, select the root container, make sure it's not expanded, and then double-click a container in the right pane. expected result: the container you double-clicked should be visible and selected in the left pane, and it's contents should be visible in the right pane.
fun fact: double click on a tag container in the right pane, and PlacesOrganizer.onPlaceSelected is called a ton of times. Do the same on a bookmark folder, and it's only called once.
Attached patch more (obsolete) — — Splinter Review
more fixes to not bother checking closed containers when we know it won't have what we're looking for. eg: excludeItems + (history or tag containers)
Attachment #375548 - Attachment is obsolete: true
Comment on attachment 375674 [details] [diff] [review] more >- openFlatContainer: function PO_openFlatContainerFlatContainer(aContainer) { >+ /** >+ * Handle double-clicks on containers in the content pane. >+ */ >+ openFlatContainer: function PO_openFlatContainer(aContainer) { >+ LOG(Date.now() + " - openFlatContainer()"); > if (aContainer.itemId != -1) >- this._places.selectItems([aContainer.itemId]); >+ this._places.selectItems([aContainer.itemId], false); This change looks unused, if possible i think we should leave this untouched and usually search before only in already open containers, and then if the node is not found do a second sweep deeper. This sounds like the approach you are applying below on FindNodes, and does not require any new param. >diff --git a/browser/components/places/content/tree.xml b/browser/components/places/content/tree.xml >--- a/browser/components/places/content/tree.xml >+++ b/browser/components/places/content/tree.xml >@@ -191,53 +191,87 @@ > --> > <method name="selectPlaceURI"> > <parameter name="placeURI"/> > <body><![CDATA[ > // Do nothing if a node matching the given uri is already selected > if (this.hasSelection && this.selectedNode.uri == placeURI) > return; > >- function findNode(container, placeURI, nodesURIChecked) { >+ var container = this.getResultNode(); >+ NS_ASSERT(container, "No result, cannot select place URI!"); >+ if (!container) >+ return; >+ >+ // A set of URIs of container-nodes that were previously searched, >+ // and thus shouldn't be searched again. This is empty at the initial >+ // start of the recursion and gets filled in as the recursion >+ // progresses. >+ var nodesURIChecked = []; >+ >+ // search for a node in a given container that matches the given >+ // place URI >+ function findNode(container, placeURI, aSearchDeep) { > var containerURI = container.uri; > if (containerURI == placeURI) > return container; >+ >+ // check if we've already searched this container > if (nodesURIChecked.indexOf(containerURI) != -1) > return null; > >+ // By default, we do search and select within containers which were >+ // closed (note that containers in which nodes were not found are >+ // closed). >+ if (aSearchDeep === undefined) >+ aSearchDeep = true; maybe would be better to invert the param, so that if undefined we will apply old behavior. Something like aSearchOnlyOpenContainers? >+ >+ // if the container is closed, and we're instructed to not open >+ // containers, return early. Note that we don't add the node to >+ // nodesURIChecked, because we haven't checked it. >+ var wasOpen = container.containerOpen; >+ var excludesItems = asQuery(container.parentResult.root).queryOptions.excludeItems; >+ var isTagContainer = PlacesUtils.nodeIsTagQuery(container); >+ var isHistoryContainer = PlacesUtils.nodeIsHost(container) || PlacesUtils.nodeIsDay(container); >+ if ((!aSearchDeep && !wasOpen && (container.parent && !container.parent.containerOpen)) || >+ (excludesItems && (isTagContainer || isHistoryContainer))) >+ return null; this condition caused me an headache :) what does (container.parent && !container.parent.containerOpen) try to check? if parent is closed and we are not going deeper, we can't be here. I don't think the check for queries is useful, since if excludeItems is true we don't even consider them containers, so i doubt we will try to open them (but please check, in case the check is needed probably this should simply check resultType = URI or VISIT and eventually check also expandQueries). >+ if (!wasOpen) >+ dump("******* opening closed container: " + [container.title, isTagContainer, excludesItems].join(",") + ") *******\n"); >+ >+ // add the current > // never check the contents of the same query > nodesURIChecked.push(containerURI); hm, supposing that this container is open, and we look into it, we mark it as checked, but if searchDeep is not defined we won't open the containers inside it. On next sweep (deep one) we will see this container has been already checked, and we won't open its child containers. We could refactor the array, do we really need to mark containers as already searched if we step into a fixed direction with a recursive method? ideally we should never come back on the same container. So on first step we could add not-yet-open containers to a searchDeeperContainerNodes array, on second sweep we apply findNodes to those only since we did already search first levels.
(In reply to comment #7) > >+ /** > >+ * Handle double-clicks on containers in the content pane. > >+ */ > >+ openFlatContainer: function PO_openFlatContainer(aContainer) { > >+ LOG(Date.now() + " - openFlatContainer()"); > > if (aContainer.itemId != -1) > >- this._places.selectItems([aContainer.itemId]); > >+ this._places.selectItems([aContainer.itemId], false); > > This change looks unused, if possible i think we should leave this untouched > and usually search before only in already open containers, and then if the node > is not found do a second sweep deeper. This sounds like the approach you are > applying below on FindNodes, and does not require any new param. yes, since we'll now search open items first, there's no win in doing this. > >+ // closed). > >+ if (aSearchDeep === undefined) > >+ aSearchDeep = true; > > maybe would be better to invert the param, so that if undefined we will apply > old behavior. > Something like aSearchOnlyOpenContainers? this param is not exposed outside of selectPlaceURI, so i don't understand what that benefit this change would get us. > this condition caused me an headache :) > what does (container.parent && !container.parent.containerOpen) try to check? > if parent is closed and we are not going deeper, we can't be here. it was intended to check if the node was visible. this is because when there's a double click on a container in the right pane, we don't want to search very deep in the left pane, just one deeper than what is visible. however, should probably just expose isVisible off of the treeview, which already tracks that, instead of trying to discern it this way. > I don't think the check for queries is useful, since if excludeItems is true we > don't even consider them containers, so i doubt we will try to open them (but > please check, in case the check is needed probably this should simply check > resultType = URI or VISIT and eventually check also expandQueries). this was specifically added because we were doing deep searches in these containers, even though we know there will be no child containers. > hm, supposing that this container is open, and we look into it, we mark it as > checked, but if searchDeep is not defined we won't open the containers inside > it. On next sweep (deep one) we will see this container has been already > checked, and we won't open its child containers. > We could refactor the array, do we really need to mark containers as already > searched if we step into a fixed direction with a recursive method? ideally we > should never come back on the same container. So on first step we could add > not-yet-open containers to a searchDeeperContainerNodes array, on second sweep > we apply findNodes to those only since we did already search first levels. yep, good catch thanks.
Blocks: 489038
Attached patch more (obsolete) — — Splinter Review
changes to the query selector bit: - checks the search target result type against the current node, and bails if there's no chance that the search target could be a sub-container. eg: no point in searching inside a date container for a tag container. - does a first pass against a node's immediate children, just checking URI, instead of depth-first. then a second pass going deep. this *really* helps in the case of tag containers, where users might have thousands of tags. - instead of tracking nodes already searched: first search all open nodes, and then search all closed nodes (per your previous comment). ignore the debugging statements and commented out code, just asking for check on this approach.
Attachment #375674 - Attachment is obsolete: true
Attachment #377988 - Flags: review?(mak77)
Attachment #377988 - Flags: review?(mak77)
Comment on attachment 377988 [details] [diff] [review] more >diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js >--- a/browser/components/places/content/places.js >+++ b/browser/components/places/content/places.js >@@ -317,17 +317,20 @@ var PlacesOrganizer = { > */ > onTreeFocus: function PO_onTreeFocus(aEvent) { > var currentView = aEvent.currentTarget; > var selectedNodes = currentView.selectedNode ? [currentView.selectedNode] : > this._content.getSelectionNodes(); > this._fillDetailsPane(selectedNodes); > }, > >- openFlatContainer: function PO_openFlatContainerFlatContainer(aContainer) { >+ /** >+ * Handle double-clicks on containers in the content pane. >+ */ since you are adding a comment, better make it full javadoc with @param desc. >@@ -191,53 +193,140 @@ > --> > <method name="selectPlaceURI"> > <parameter name="placeURI"/> > <body><![CDATA[ > // Do nothing if a node matching the given uri is already selected >+ // search for a node in a given container that matches the given >+ // place URI >+ function findNode(container, aSearchDeep) { > var containerURI = container.uri; > if (containerURI == placeURI) > return container; >+ >+ LOG("findNode(" + containerURI + ")"); >+ /* >+ // check if we've already searched this container > if (nodesURIChecked.indexOf(containerURI) != -1) > return null; >+ */ > >- // never check the contents of the same query >- nodesURIChecked.push(containerURI); >+ // By default, we do search and select within containers which were >+ // closed (note that containers in which nodes were not found are >+ // closed). >+ if (aSearchDeep === undefined) >+ aSearchDeep = true; >+ >+ // Searching for tag container, cannot be a subcontainer (deep or >+ // not) of a date container. >+ if (options.resultType == options.RESULTS_AS_TAG_CONTENTS && >+ (PlacesUtils.nodeIsHost(container) || PlacesUtils.nodeIsDay(container))) >+ return null; >+ >+ // Searching for any non-tag container, cannot be a subcontainer >+ // (deep or not) of a tag query. >+ var queryOptions = asQuery(container.parentResult.root).queryOptions; >+ if (options.resultType != options.RESULTS_AS_TAG_CONTENTS && >+ queryOptions.resultType == options.RESULTS_AS_TAG_QUERY) >+ return null; what about moving all of this into a canContainNode util? >+ // if we're not doing a deep search and the node is not visible >+ // OR the node has no searchable subcontainers, then don't search >+ // it's contents. >+ >+ // Note that we don't add the node to nodesURIChecked, because we >+ // haven't checked it. >+ >+ var wasOpen = container.containerOpen; >+ var hasNoSubContainers = PlacesUtils.nodeIsTagQuery(container) || >+ PlacesUtils.nodeIsHost(container) || >+ PlacesUtils.nodeIsDay(container); what about a nodeIsContainersQuery util? >+ var isHidden = container.parent && container.viewIndex == -1; >+ if ((!aSearchDeep && (!wasOpen || isHidden)) || >+ (queryOptions.excludeItems && hasNoSubContainers)) { >+ if (!aSearchDeep && !wasOpen) >+ closedNodes.push(container); >+ return null; >+ } this condition is a bit unreadable, better having some more temp bool var > > var wasOpen = container.containerOpen; > if (!wasOpen) > container.containerOpen = true; >+ >+ // breadth-first search for match on child URIs. > for (var i = 0; i < container.childCount; ++i) { > var child = container.getChild(i); >- var childURI = child.uri; >- if (childURI == placeURI) >+ if (child.uri == placeURI) > return child; >- else if (PlacesUtils.nodeIsContainer(child)) { >- var nested = findNode(asContainer(child), placeURI, nodesURIChecked); >+ } >+ >+ LOG("findNode(" + containerURI + "): searching deep inside"); >+ // deep search of children that are containers >+ for (var i = 0; i < container.childCount; ++i) { >+ var child = container.getChild(i); >+ if (PlacesUtils.nodeIsContainer(child)) { >+ var nested = findNode(asContainer(child), aSearchDeep); > if (nested) > return nested; > } > } I'm not sure about the potential of splitting these 2 loops, they're looping on the same datas, looks like a perf hit, i see this checks uris first and containers later, but it's still iterating 2 times the data. Do we have numbers that indicate this gives back a good perf gain? Maybe the first loop could cache containers and the second loop could go through them, so it will automatically skip all non-containers. "var nested" name is confusing, could you find a meaningful name? maybe foundNode? Globally the search algo looks much more "smart" then before, and i think this is going toward the right direction.
Whiteboard: [TSnappiness] → [TSnappiness][needs new patch]
Flags: wanted-firefox3.5?
Flags: wanted-firefox3.5? → wanted-firefox3.5+
Flags: wanted-firefox3.5+ → wanted1.9.1.x?
Blocks: 405765
status1.9.1: --- → ?
Flags: wanted1.9.1.x?
Attached patch unrotted — — Splinter Review
hrm, i wonder if this conflicts with mano's lazy tree patch...
Attachment #377988 - Attachment is obsolete: true
Assignee: dietrich → nobody
I didn't see anymore reports of slow openings from the right pane, and regardless the patch is heavily bitrotted. It contained some good ideas, so eventually we may reuse them in new code.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: