User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.68 Safari/537.36 Steps to reproduce: My extension, SmartSearch, calls getKeywordForBookmark for each of the user's bookmarks. (I admit that there may be better ways to do this in bulk. I should probably look into that.) Actual results: The operation, which used to happen almost instantly, now takes much longer. Most users have reported delays of 3-5 seconds, but I see ~20 seconds, and another user said the problem was "permanent" (but perhaps he just got tired of waiting). Presumably the exact length of time depends on the number of bookmarks. (To determine which part of SmartSearch was responsible, I added some naive "new Date()"-based profiling. It identified getKeywordForBookmark as responsible for virtually the entire runtime.) https://addons.mozilla.org/en-US/firefox/addon/smartsearch/reviews/523811/ https://addons.mozilla.org/en-US/firefox/addon/smartsearch/reviews/526326/ Expected results: The operation should complete quickly :) I speculate that the problem is in this change to nsNavBookmarks, which is present in release 26 and in trunk. (Things worked fine in release 25.) http://hg.mozilla.org/mozilla-central/diff/bc427f5ec61b/toolkit/components/places/nsNavBookmarks.cpp The method that is being changed is a method that builds a cache of bookmark-keyword data. That method is called during each bookmark-keyword query. The problem is that the change seems to have removed the "Has the cache been built?" check. The change was part of a nsTHashtable cleanup, and it amounts to changing this... if (mHash.IsInitialized()) return; mHash.Init(); ActuallyPopulateTheCache(); ...to this... ActuallyPopulateTheCache(); While it makes sense that there's no need to check IsInitialized() and call Init() on the hash (as initialization is apparently automatic nowadays(?)), the old IsInitialized() call had a second purpose: to make sure that the cache was built only once. Assuming that I have that right, the fix is as simple as manually tracking the initialization status: if (mHashInitialized) return; mHashInitialized = true; ActuallyPopulateTheCache();
(I found a workaround: My extension will now make a SQL query for all bookmarks at once. That said, the bulk query feels a hair slower than the old cached queries, so I'd still be happy to see this fixed.)
Robert, you wrote the patch for bug 910989. Does the below make sense to you? I don't know enough about nsHashTable and places to fully grasp the problem/patch here...
Created attachment 8355424 [details] [diff] [review] fix Thanks for catching this!
Comment on attachment 8355424 [details] [diff] [review] fix Thanks!
Comment on attachment 8355424 [details] [diff] [review] fix [Approval Request Comment] Bug caused by (feature/regressing bug #): 910989 User impact if declined: potentially very slow behavior in some bookmarks-related addons Testing completed (on m-c, etc.): just landed Risk to taking this patch (and alternatives if risky): very low risk. Basically just restores previous behavior. String or IDL/UUID changes made by this patch: none
Chris, can you please verify this is working for you now in Nightly, Aurora, and Beta?
Thanks very much for the fix! Fixed in nightly and Aurora (firefox-29.0a1.en-US.linux-x86_64 and firefox-28.0a2.en-US.linux-i686). Still slow in Beta (firefox-27.0b4 -- is that the right one to be testing?).
(In reply to Chris Povirk from comment #10) > Thanks very much for the fix! Thanks for the clear bugreport! > Fixed in nightly and Aurora (firefox-29.0a1.en-US.linux-x86_64 and > firefox-28.0a2.en-US.linux-i686). > > Still slow in Beta (firefox-27.0b4 -- is that the right one to be testing?). No, the fix will only be in b5, which was produced just yesterday (might not be available through auto-update yet, I'm not sure). Either wait for it to be available through the update functionality, or just manually untar-bz the file in http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/latest-beta/linux-i686/en-US/ somewhere and try that. :-)
Aha! Thanks for the information and the link. It does indeed work in b5.
(In reply to Chris Povirk from comment #12) > Aha! Thanks for the information and the link. It does indeed work in b5. Excellent, thanks for confirming! :-)