EnsureKeywordsHash no longer takes advantage of caching

VERIFIED FIXED in Firefox 27

Status

()

Toolkit
Places
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Chris Povirk, Assigned: roc)

Tracking

26 Branch
mozilla29
Points:
---

Firefox Tracking Flags

(firefox27 verified, firefox28 verified, firefox29 verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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();
(Reporter)

Comment 1

4 years ago
(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.)
OS: Linux → All
Hardware: x86_64 → All

Comment 2

4 years ago
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...
Blocks: 910989
Component: Untriaged → Places
Flags: needinfo?(roc)
Product: Firefox → Toolkit
Created attachment 8355424 [details] [diff] [review]
fix

Thanks for catching this!
Assignee: nobody → roc
Attachment #8355424 - Flags: review?(mano)
Flags: needinfo?(roc)
Comment on attachment 8355424 [details] [diff] [review]
fix

Thanks!
Attachment #8355424 - Flags: review?(mano) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/90d56ff72f1a
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
Attachment #8355424 - Flags: approval-mozilla-beta?
Attachment #8355424 - Flags: approval-mozilla-aurora?

Comment 7

4 years ago
https://hg.mozilla.org/mozilla-central/rev/90d56ff72f1a
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Attachment #8355424 - Flags: approval-mozilla-beta?
Attachment #8355424 - Flags: approval-mozilla-beta+
Attachment #8355424 - Flags: approval-mozilla-aurora?
Attachment #8355424 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/fac124f2dbb4
https://hg.mozilla.org/releases/mozilla-beta/rev/869f12d5b4d6
status-firefox27: --- → fixed
status-firefox28: --- → fixed
status-firefox29: --- → fixed
Chris, can you please verify this is working for you now in Nightly, Aurora, and Beta?
Flags: needinfo?(beigetangerine)
(Reporter)

Comment 10

4 years ago
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

4 years ago
(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. :-)
status-firefox28: fixed → verified
status-firefox29: fixed → verified
(Reporter)

Comment 12

4 years ago
Aha! Thanks for the information and the link. It does indeed work in b5.
Flags: needinfo?(beigetangerine)

Comment 13

4 years ago
(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! :-)
Status: RESOLVED → VERIFIED
status-firefox27: fixed → verified
You need to log in before you can comment on or make changes to this bug.