656188 - Cache last 10 fetched bookmarks info to speed up repeated requests

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

this can help Sync, transactions manager and bookmarks dialogs till we put together a new API, since most of the getters after bug 633274 go through a single getter method, it can easily manage a cache of recent fetches.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Was easier than I thought, not asking review yet, till the dependency is fixed.

2 downsides to this:
- Changes to bookmarks service need a bit more attention. Mostly any method changing something has to invalidate the cache (this comes almost for free now, but future changes must take this into account)
- Direct writes to the database may confuse the cache. But we don't expect anyone to directly UPDATE or DELETE from moz_bookmarks.

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.1

no hurry on this.
I think I can't write a better test for this than the >150 tests we already have.

Comment 3

[Marco Bonardo [:mak]]

PS: I had to change the test because it's doing a direct write on the db, clearly the cache becomes confused.

Comment 4

[Marco Bonardo [:mak]]

As an idea for a future notification API, we could notify just things like onItemRemoved(guid), onItemAdded(guid) and store a timed cache (like 5 minutes) of the item, the listener could use any getter api (getTitle, getURI) in the notification based on his needs, hitting our internal cache.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 532347 [details] [diff] [review]
patch v1.1

Review of attachment 532347 [details] [diff] [review]:
-----------------------------------------------------------------

r=sdwilsh

::: toolkit/components/places/nsNavBookmarks.cpp
@@ +208,5 @@
> +{
> +  if (hashTable->Count() > RECENT_BOOKMARKS_INITIAL_CACHE_SIZE) {
> +    PRInt64 threshold = PR_Now() - RECENT_BOOKMARKS_THRESHOLD;
> +    hashTable->Enumerate(ExpireNonrecentBookmarksCallback,
> +                         reinterpret_cast<void*>(&threshold));

Doesn't Enumberate return something?  Either check it or cast to void please.

::: toolkit/components/places/nsNavBookmarks.h
@@ +209,3 @@
>     */
>    nsresult FetchItemInfo(PRInt64 aItemId,
> +                         mozilla::places::BookmarkData& _bookmark,

I thought we did typedef mozilla::places::BookmarkData BookmarkData; so we didn't need to include this here.

@@ +573,5 @@
> +  /**
> +   * Cache for the last fetched BookmarkData entries.
> +   * This is used to speed up repeated requests to the same item id.
> +   */
> +  nsDataHashtable<nsTrimInt64HashKey, mozilla::places::BookmarkData> mBookmarksInfoCache;

I'd really prefer us to use nsTHashTable here.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Addressed all comments

Comment 7

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/504270705d88

Comment 8

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/504270705d88

marking in-testsuite since this code is leveraged by most of the existing tests.

Comment 9

[Daniel C]

Will this be used when using the awesomebar? Say, typing * to get recent bookmarks, or + foo to get bookmarks with the tag "foo"?

If so, would it make sense to increase the number of cached items from 10 to 12 as that is the number of items that appears in the dropdown?

Sorry if I've completely misunderstood this cache.

Comment 10

[Marco Bonardo [:mak]]

no, the awesomebar follows another path.
This will be useful for Sync and for bookmarks organization (remove, move, drag&drop, copy&paste and such).

Comment 11

[Marco Bonardo [:mak]]

and for add-ons using the bookmarks API