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.

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.

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.

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.

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.

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.