Closed Bug 417729 Opened 14 years ago Closed 11 years ago

Livemark refreshing inefficient - repeatedly requests parent annotation which is known

Categories

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

defect

Tracking

()

RESOLVED WORKSFORME
Firefox 3.6a1

People

(Reporter: ondrej, Unassigned)

References

Details

(Keywords: perf, Whiteboard: [tsnap])

When livemark node is refreshed, it populates children in LivemarkLoadListener.runBatched and inserts them into database using LivemarkLoadListener.insertLivemarkChild. However this method uses generic nsINavBookmarksService::insertBookmark which will check for each item whether parent is livemark or not. This calls nsILivemarkService::IsLivemark on the parent folder for each item.

So it all looks like JS->native->JS->native call, which can be reduced to JS->native by introducing new method nsINavBookmarksService::insertLivemarkChild method.

This would reduce number of queries performed regularly when refreshing livemarks and should generally speed up work with Firefox.

This regression has been introduced with bug 394038.
Flags: blocking-firefox3?
OS: Windows XP → All
Hardware: PC → All
I do not want to add livemark-specific optimizations to the bookmarks service. I expect there'll be a plethora of "applications" such as livemarks, microsummaries, etc built using the bookmarks+annotations combination. We need to keep the infrastructure generically capable of accommodating this usage pattern.

Regarding frecency calculation, here are 2 thoughts:

1. We probably don't need to do it synchronously. Maybe we should move it to a lazy model like we add history visits.

2. I don't like having application specificity like IsQuery/IsLivemarkChild/IsParentATagFolder this deep in the backend. Maybe we should have a generic FrecencyAmplifier helper class, which is registered with the history service, and called when calculating frecency. This would also allow extensions to amplify frecency (eg: a URI is tagged on del.icio.us, so should be bumped up).
Ok, we do not need to add livemark specific optimization, we just need to fix the way annotations are read. In this case this would mean passing a reference to the parent instead of its ID, and asking it for the annotation and grand parent id instead of calling GetFolderIdForItem.

So this bug could be changed to something like: nsNavBookmarks::InsertBookmarks needs optimization to reduce database requests.
more optimizations:

It might be unnecessary to do the IsLivemarkItem check here at all, because we could filter those items from autocomplete later using the list of livemark feed ids kept by the autocomplete code for determining which favicon to use.

Also, while determining IsBookmark in this code, we get the URI spec and see if it's a query URI. Using nsIURI.schemeIs() might be faster:

http://www.xulplanet.com/references/xpcomref/ifaces/nsIURI.html#method_schemeIs
Not blocking, but definitely wanted. (Thanks, Ondrej, for doing all this analysis!)
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Depends on: 420078
Assignee: ondrej → nobody
Priority: -- → P2
Target Milestone: --- → Firefox 3.2a1
Whiteboard: [tsnap]
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
The overhead here is not that interesting right now, the check is done in a subquery, no xpcom boundaries are crossed, and the right solution is to make livemarks not bookmarks.
WFM due to previous changes.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.