Maintaining a result is expensive because it keeps itself updated and may have a lot of children. When switching places views, these results can hang around for a bit before they get garbage collected, slowing everything down (and making debugging confusing). Add a function on a result to destroy it which will destroy its children and unhook all observers. Then we don't really care when the actual result object gets garbage collected.

I don't think that destroying all children is useful, what we need is stop being notified. All the rest can be easily done by the cycle collector.

Comment on attachment 379133 [details] [diff] [review] patch v1.0 >diff --git a/toolkit/components/places/public/nsINavHistoryService.idl b/toolkit/components/places/public/nsINavHistoryService.idl >--- a/toolkit/components/places/public/nsINavHistoryService.idl >+++ b/toolkit/components/places/public/nsINavHistoryService.idl >@@ -616,8 +616,14 @@ interface nsINavHistoryResult : nsISuppo > * containers for their contents to be valid. > */ > readonly attribute nsINavHistoryContainerResultNode root; >+ >+ /** >+ * When a result go out of scope it will continue to observe changes till it >+ * is cycle collected. This method allows to explicitely mark result as out >+ * of scope, since updating results in some situation could be expensive. >+ */ >+ void stopObserving(); > }; update UUID s/go/goes/ s/till/until/ s/to explicitely/the caller to explicitly the/ >+NS_IMETHODIMP >+nsNavHistoryResult::StopObserving() >+{ >+ if (mIsBookmarkFolderObserver || mIsAllBookmarksObserver) { >+ nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService(); >+ if (bookmarks) { >+ bookmarks->RemoveObserver(this); >+ mIsBookmarkFolderObserver = PR_FALSE; >+ mIsAllBookmarksObserver = PR_FALSE; >+ } >+ } >+ if (mIsHistoryObserver) { >+ nsNavHistory* history = nsNavHistory::GetHistoryService(); >+ if (history) { >+ history->RemoveObserver(this); >+ mIsHistoryObserver = PR_FALSE; >+ } >+ } >+ >+ return NS_OK; >+} either check the result of the RemoveObserver() calls, or (void). so, there's the comments on the patch. however, i'm not sold on the approach: - this makes life more complicated for callers. this scenario should be performant for the default case, otherwise extension developers will hurt us. - there's gotta be a way to be notified or detect when the result goes out of scope. who can we talk to in JS to see what's possible here? - i'd prefer more flexible control, such as a getter/setter for IsObserving or something like that. ideally it could also be configurable by query options so one could create a result that never live-updates.

> - this makes life more complicated for callers. this scenario should be > performant for the default case, otherwise extension developers will hurt us. They are already, this change is a "you should do this", but it's not a "must do this". The change is mostly intended for our internal use, but clearly anyone using it would be appreciated. So this allows callers to be gentle toward us, but nothing changes really if they don't. > - there's gotta be a way to be notified or detect when the result goes out of > scope. who can we talk to in JS to see what's possible here? I'd really like that, but i can't see how, when a js var goes out of scope it will be wiped by the garbage collector, but since it's hard to tell when that will happen we would end up back with the same issue. Our results are already garbaged automatically, but they take too long for some case (see tagging service). Probably even if we would detect when the js var is garbage collected it would be too late. > - i'd prefer more flexible control, such as a getter/setter for IsObserving or > something like that. ideally it could also be configurable by query options so > one could create a result that never live-updates. it's hard to go back observing once stopObserving is called since the result is completely out of sync. We could also support such a query options, that would not solve one of the issues though, would be optimal for our tagging service or utils issues, but would not work for library searches, or the sidebar, where we want to continue updating the result till the search is replaced. I'm actually thinking maybe we could do this internally when root.containerOpen = false is called, that would be cleaner and more understandable, would still require implementer to recall to close the root node, but would be easier to inject as a good Places programming technique.

This is an alternative approach, where result stops observing if root node is closed. Would this be better? The only different behavior is that when the above happens i have to clear children of the node, so that when it's reopened it will be refreshed (instead of being refreshed while it is closed). The only problem i've found so far is with result.sortingMode for containersQueries, looks like if i close root node, set sortingMode on the result, and reopen root node, some sortingMode is not applied to children. Btw i don't see many cases where one needs to close root node, set sortingMode, and reopen it, that was just a choice in the test flow. Eventually this could be a followup, since i've not yet found what's happening.

I think this approach overcomes patch v1.0, closing something you have opened is almost "natural" to do. This fixes the above sortingMode issue.

Comment on attachment 382602 [details] [diff] [review] patch v2.1 this looks fine, r=me. much better approach, thanks.

adding Tsnap since this could potentially avoid some of the hangs in the UI (those happening when a slow result is still updating in memory even if not used, eg: sidebar, Library right pane)