lazily init the bookmark uri hash

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dietrich, Assigned: dietrich)

Tracking

({perf})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tsnap])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 381816 [details] [diff] [review]
wip

we fill the bookmark uri hash when initializing the bookmark service, but don't access it during the startup path, unless migrating (ie: once ever, for the common case).

time to init the hash in my local tests:

empty profile: 1ms
my big profile: 650ms (!!!)

the attached patch changes consumers of it to check and init if needed. there's probably a fancier way to do this, but it gets the job done.

asking for review of the approach.
Attachment #381816 - Flags: review?(mak77)
(Assignee)

Updated

9 years ago
Whiteboard: [TStartup]
(Assignee)

Comment 1

9 years ago
Created attachment 381839 [details] [diff] [review]
wip

passes tests
Assignee: nobody → dietrich
Attachment #381816 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #381839 - Flags: review?(mak77)
Attachment #381816 - Flags: review?(mak77)
(Assignee)

Comment 2

9 years ago
would take review from sdwilsh as well. makes a non-trivial difference in startup time, especially for folks w/ lots of history and bookmarks (aka everyone running firefox 3, wrt to history).
Attachment #381839 - Flags: review?(mak77)
Comment on attachment 381839 [details] [diff] [review]
wip

Lazily initing the hash is for sure a good choice, could you please investigate which action is most likely to cause it to be inited with this change? I think we should start having an idea of what action this slowdown will hit if not Ts.

Also we should look into fillBookmarksHash since we must also make that faster if possible. i see the first query is quite strange:
"SELECT h.id "
"FROM moz_bookmarks b "
"LEFT JOIN moz_places h ON b.fk = h.id where b.type = ?1"),
why is this not simply "SELECT b.fk FROM moz_bookmarks WHERE b.type = ?1" ? this looks like a useless JOIN, and will return also a bunch of NULLs. Maybe we did lost part of the query with changes? (need to investigate in mxr).

The second query (UNIONs) looks slower due to partitioned tables, but i'm not sure we can make it faster, i think this is what gives you those 650ms (but also walking all results in the first one won't be so fast).

Actually i'm thinking we could simply UNION results from this query to the other query, and execute only one query and one looping.


>@@ -1340,17 +1356,17 @@ nsNavBookmarks::CreateContainerWithID(PR
>     rv = statement->BindInt64Parameter(6, aItemId);
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
>   rv = statement->BindUTF8StringParameter(0, aName);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   PRInt32 containerType =
>-    aIsBookmarkFolder ? TYPE_FOLDER : TYPE_DYNAMIC_CONTAINER;
>+    aIsBookmarkFolder ? (PRInt32)TYPE_FOLDER : (PRInt32)TYPE_DYNAMIC_CONTAINER;
> 
>   rv = statement->BindInt32Parameter(1, containerType);

hm, does this warn? it should cast PRUint16 to PRInt32, btw i would rather make containerType PRUint16

>+  if (!mBookmarksHash.IsInitialized()) {
>+    printf("BOOKMARKHASH: IsBookmarked init'd the hash\n");
>+    nsresult rv = FillBookmarksHash();
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }

i guess if would not be better to have a getter or macro like GetBookmarksHash() that will check IsInitialized and eventually call FillBookmarksHash, rather than repeating the check everywhere.
(Assignee)

Comment 4

9 years ago
(In reply to comment #3)
> (From update of attachment 381839 [details] [diff] [review])
> Lazily initing the hash is for sure a good choice, could you please investigate
> which action is most likely to cause it to be inited with this change? I think
> we should start having an idea of what action this slowdown will hit if not Ts.

http://mxr.mozilla.org/mozilla-central/search?string=getbookmarkedurifor

called by the favicon service

about the query changes: i'll file separate bugs for those

> >   PRInt32 containerType =
> >-    aIsBookmarkFolder ? TYPE_FOLDER : TYPE_DYNAMIC_CONTAINER;
> >+    aIsBookmarkFolder ? (PRInt32)TYPE_FOLDER : (PRInt32)TYPE_DYNAMIC_CONTAINER;
> > 
> >   rv = statement->BindInt32Parameter(1, containerType);
> 
> hm, does this warn? it should cast PRUint16 to PRInt32, btw i would rather make
> containerType PRUint16

it's a code change unrelated to this patch, that fixes a current warning. i'll file a separate bug for it..
(Assignee)

Comment 5

9 years ago
Created attachment 382243 [details] [diff] [review]
v1

added a getter, removed the unrelated code. will remove the debug stuff before check-in.
Attachment #381839 - Attachment is obsolete: true
Attachment #382243 - Flags: review?(mak77)
Comment on attachment 382243 [details] [diff] [review]
v1

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 381839 [details] [diff] [review] [details])
> http://mxr.mozilla.org/mozilla-central/search?string=getbookmarkedurifor
> 
> called by the favicon service

so sounds like it is called on first page load. This change should be a win for Fennec too.

>diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp

>@@ -648,29 +645,42 @@ nsNavBookmarks::CreateRoot(mozIStorageSt
>   rv = insertStatement->BindInt64Parameter(1, *aID);
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = insertStatement->Execute();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return NS_OK;
> }
> 
>+// nsNavBookmarks::GetBookmarksHash
>+//
>+//    Getter and lazy initializer of the bookmarks redirect hash.
>+//    See FillBookmarksHash for more information.
>+
>+nsDataHashtable<nsTrimInt64HashKey, PRInt64>*
>+nsNavBookmarks::GetBookmarksHash()
>+{
>+  if (!mBookmarksHash.IsInitialized())
>+    (void)FillBookmarksHash();
>+  return &mBookmarksHash;
>+}

The only concern i have about this is it is ignoring errors, but that method is quite simple, it could actually fail only if the queries are wrong, and that would most likely be catched by unit tests (do we have a unit test covering this?).
Fine if a unit test is present or possible to check that FillBookmarksHash works correctly, otherwise you should either return rv and have an out param, or have a rv param.
Or even easier, make it assert / warn in debug mode in case it does not succeed. something like
{
  nsresult rv = FillBokmarksHash();
  NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv));
}
Due to the nature of the method, if we catch the error in debug mode we should be fine, so i'd go for it.

Also I think this should be called GetBookmarksHash if all mBookmarksHash (excluded those in FillBookmarksHash that now is an helper for the getter) are replaced with the getter, otherwise it works more like a EnsureBookmarksHash. Should be clear to any new contributor that he should not directly use mBookmarksHash without first ensuring it is initialized.

> // nsNavBookmarks::FillBookmarksHash
> //
> //    This initializes the bookmarks hashtable that tells us which bookmark
> //    a given URI redirects to. This hashtable includes all URIs that
> //    redirect to bookmarks.
> //
> //    This is called from the bookmark init function and so is wrapped
> //    in that transaction (for better performance).

The above is no more true, btw this is a select so won't gain much perf from a transaction.

>+  printf("BOOKMARKHASH: UpdateBookmarkHashOnRemove\n");
>+  (void)GetBookmarksHash();
>+
>   // remove it
>   mBookmarksHash.Enumerate(RemoveBookmarkHashCallback,
>                            reinterpret_cast<void*>(&aPlaceId));
>   return NS_OK;
> }

Why is this different from the others (GetBH()->Enum)?
This is what i meant before talking about difference between getBH and EnsureBH, we must be sure to follow one direction everywhere to make it less error-prone, imo.

>diff --git a/toolkit/components/places/src/nsNavBookmarks.h b/toolkit/components/places/src/nsNavBookmarks.h
>--- a/toolkit/components/places/src/nsNavBookmarks.h
>+++ b/toolkit/components/places/src/nsNavBookmarks.h
>@@ -161,17 +161,21 @@ private:
>   PRInt32 mBatchLevel;
> 
>   // true if the outermost batch has an associated transaction that should
>   // be committed when our batch level reaches 0 again.
>   PRBool mBatchHasTransaction;
> 
>   // This stores a mapping from all pages reachable by redirects from bookmarked
>   // pages to the bookmarked page. Used by GetBookmarkedURIFor.
>+  typedef nsDataHashtable<nsTrimInt64HashKey, PRInt64> PRInt64Hash;
>+  //PRInt64Hash mBookmarksHash;
>+  //PRInt64Hash* GetBookmarksHash();
>   nsDataHashtable<nsTrimInt64HashKey, PRInt64> mBookmarksHash;
>+  nsDataHashtable<nsTrimInt64HashKey, PRInt64>* GetBookmarksHash();

Recall to fix those before final

r=me with the above fixed
Attachment #382243 - Flags: review?(mak77) → review+
(Assignee)

Comment 7

9 years ago
Created attachment 382329 [details] [diff] [review]
for checkin

comments fixed
Attachment #382243 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Whiteboard: [TStartup] → [tsnap][has review][can land]
(Assignee)

Comment 8

9 years ago
trunk: http://hg.mozilla.org/mozilla-central/rev/889f3a64face
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Whiteboard: [tsnap][has review][can land] → [tsnap]

Updated

9 years ago
Depends on: 531236
You need to log in before you can comment on or make changes to this bug.