955889 - EnsureKeywordsHash no longer takes advantage of caching

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Chris Povirk]

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();

Comment 1

[Chris Povirk]

(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.)

Comment 2

[:Gijs (he/him)]

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...

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Thanks for catching this!

Comment 4

[Asaf Romano (gone)]

Comment on attachment 8355424 [details] [diff] [review]
fix

Thanks!

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90d56ff72f1a

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/90d56ff72f1a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fac124f2dbb4
https://hg.mozilla.org/releases/mozilla-beta/rev/869f12d5b4d6

Comment 9

[u279076]

Chris, can you please verify this is working for you now in Nightly, Aurora, and Beta?

Comment 10

[Chris Povirk]

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?).

Comment 11

[:Gijs (he/him)]

(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. :-)

Comment 12

[Chris Povirk]

Aha! Thanks for the information and the link. It does indeed work in b5.

Comment 13

[:Gijs (he/him)]

(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! :-)