Closed Bug 542943 Opened 10 years ago Closed 10 years ago

Get rid of bookmarksHash

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a3

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [Ts][patch applies on top of bug 542941])

Attachments

(1 file, 1 obsolete file)

This hash has been added to fast check isBookmark, or redirects from bookmarks. It is expensive to maintain, and the gain we obtain feels like not existent. I've measured isBookmarked with and without the hash, and the version without the hash was faster by 50ms (calling it for 1000 bookmarks)
It does not even save us from I/O on the main-thread, since even if we don't query the DB for the bookmark we query it for the placeId, so there is no real difference.
I have a patch but while working on it i've hit some strange thing in history, that i'm following in bug 542941.
I also need to get a Talos Tp4 measure with this change.
Depends on: 542941
Blocks: 540765
This is a Ts bug, i doubt will be visible in Ts Talos tests, but should be good for real-life and mobile, since we won't anymore have to query all bookmarks at startup to build the hash.
Flags: in-testsuite?
Whiteboard: [Ts]
OS: Windows 7 → All
Hardware: x86 → All
Attached patch patch v1.0 (obsolete) — Splinter Review
Ideally getBookmarkedURIFor should also be async, with a callback, but i plan to check it back after favicons service work, since this is being called by favicons service for each added icon. Notice that even if actually we query the db instead of the hash table, previously we were still hitting the db to convert the url to a place_id, so the only possible future road for this method is async, but i could even end up exposing the query and directly using it from favicons service.
Attachment #425523 - Flags: review?(dietrich)
Whiteboard: [Ts] → [Ts][patch applies on top of bug 542941]
Comment on attachment 425523 [details] [diff] [review]
patch v1.0

>@@ -429,6 +391,85 @@ nsNavBookmarks::GetStatement(const nsCOM
>   RETURN_IF_STMT(mDBChangeBookmarkURI, NS_LITERAL_CSTRING(
>     "UPDATE moz_bookmarks SET fk = ?1, lastModified = ?2 WHERE id = ?3 "));
> 
>+  // The next query finds the bookmarked ancestors in a redirects chain.
>+  // It won't go upper than 3 levels of redirects (a->b->c->your_place_id).

s/upper/further/

>+  // To make this path 100% correct (up to any level) we would need either:
>+  //  - A separate hash, build through recursive querying of the database.
>+  //    This solution was previously implemented, but it had a negative effect
>+  //    on startup since at each startup we have to recursive query the database

recursively

>+  //    to rebuild an hash that is always the same across sessions.  It must be

s/an/a/

>+  //    updated at each visit and bookmarks change too.  The code to manage it
>+  //    is complex and prone to errors, bringing easily to return the wrong

s/bringing easily to return.../sometimes returning/

or something like that.

>+    "FROM moz_historyvisits dst "
>+    "JOIN moz_bookmarks b ON b.fk = " COALESCE_PLACEID
>+    "LEFT JOIN moz_historyvisits src ON src.id = dst.from_visit "
>+    "LEFT JOIN moz_historyvisits grandfather ON src.from_visit = grandfather.id "
>+      "AND src.visit_type IN (") + redirectsFragment + NS_LITERAL_CSTRING(") "
>+    "LEFT JOIN moz_historyvisits ancestor ON grandfather.from_visit = ancestor.id "
>+      "AND grandfather.visit_type IN (") + redirectsFragment + NS_LITERAL_CSTRING(") "
>+    "WHERE dst.visit_type IN (") + redirectsFragment + NS_LITERAL_CSTRING(") "
>+      "AND dst.place_id = ?1 "
>+    "LIMIT 1 " // Stop at the first result.
>+  ));

for this query, instead of dst/src/grandfather/ancestor, i'd prefer something more coherent like visits1 - visits4, or self/parent/grandparent/greatgrandparent.

>+/**
>+ * Adds a fake redirect between two visits.
>+ */
>+function addFakeRedirect(aSourceVisitId, aDestVisitId, aRedirectType) {
>+  let dbConn = DBConn();
>+  let stmt = dbConn.createStatement(
>+    "UPDATE moz_historyvisits "+

nit: space after quote
Comment on attachment 425523 [details] [diff] [review]
patch v1.0

r=me. make sure to add dev-doc-needed once checked in, so that the redirection cap is noted in the docs.
Attachment #425523 - Flags: review?(dietrich) → review+
Attached patch patch v1.1Splinter Review
Attachment #425523 - Attachment is obsolete: true
what's the status of this? ready to be checked-in?
the patch is built on top of bug 542941, that is waiting for a review and a SR.
Blocks: 487003
Blocks: 447581
http://hg.mozilla.org/mozilla-central/rev/6fead70c1560
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a3
Is it practical to backport this to 3.6 in the same time frame as Lorentz?
(In reply to comment #9)
> Is it practical to backport this to 3.6 in the same time frame as Lorentz?
I think, because it's on top of bug 542941, that it may not be such a good idea.  We may be able to split the two apart, but we have a lot of other important work we need to get done too.
Blocks: 561148
Blocks: 462461
You need to log in before you can comment on or make changes to this bug.