We should use a cloned connection check if a URI is visited or not in mozilla::places::History. We can generate a whole bunch of requests, and I'd rather us not overload the background thread. It also adds more lock contention to the database connection when it's all done on one connection.

This might actually work, but I'm on top of bug 582703 which isn't currently passing tests. Tomorrow I'll likely reverse the ordering because that bug is actually harder than this one...

Rebased to not depend on that EVIL bug! :) This was, in fact, rather trivial to do. Passes make check and make xpcshell-tests for places locally.

Comment on attachment 481225 [details] [diff] [review] v1.0 >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >--- a/toolkit/components/places/src/History.cpp >+++ b/toolkit/components/places/src/History.cpp >@@ -172,7 +172,7 @@ public: > // If we are a content process, always remote the request to the > // parent process. > if (XRE_GetProcessType() == GeckoProcessType_Content) { >- mozilla::dom::ContentChild * cpc = >+ mozilla::dom::ContentChild * cpc = while fixing the space, please also fix pointer style toward type. >+History::GetIsVisitedStatement() >+{ >+ // If we already have a cached statement, return now! >+ if (mIsVisitedStatement) { >+ return mIsVisitedStatement; >+ } is the comment above really useful? the code is self explaining >+ // At this point, we couldn't have a cached statement, so create it now. I guess at this point we don't have a cached statement for sure >diff --git a/toolkit/components/places/src/History.h b/toolkit/components/places/src/History.h >--- a/toolkit/components/places/src/History.h >+++ b/toolkit/components/places/src/History.h >@@ -48,6 +48,7 @@ > #include "nsTArray.h" > #include "nsDeque.h" > #include "nsIObserver.h" >+#include "mozIStorageConnection.h" nit: maybe makes sense to use mozilla/storage.h for future changes to History.cpp (removing steps etc)? > /** > * Obtains a pointer that has had AddRef called on it. Used by the service nit: "that is AddRefed"? >+ /** >+ * An asynchronous statement to query if a URI is visited or not. >+ */ >+ nsCOMPtr<mozIStorageAsyncStatement> mIsVisitedStatement; add a @note to always get it through the getter, and not directly use it. >- typedef nsTArray<mozilla::dom::Link *> ObserverArray; >+ typedef nsTArray<mozilla::dom::Link* > ObserverArray; the space after the * looks useless >diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp >- // Lock the database file. This is done partly to avoid third party >- // applications to access it while it's in use, partly for performance. >- rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( >- "PRAGMA locking_mode = EXCLUSIVE")); >- NS_ENSURE_SUCCESS(rv, rv); we need Talos numbers on this change, maybe you could do a quick Tryserver run.

(In reply to comment #3) > nit: maybe makes sense to use mozilla/storage.h for future changes to > History.cpp (removing steps etc)? Avoiding it in the .h file because mozilla/storage.h includes a bunch and that will hurt compile times. I'm all for using it in the .cpp file. > > * Obtains a pointer that has had AddRef called on it. Used by the service > > nit: "that is AddRefed"? Would prefer AddRef since that is the actual method name and is therefore clearer. > we need Talos numbers on this change, maybe you could do a quick Tryserver run. Was planning on landing it on the places branch. We can always backout if it's a regression, but I'm pretty sure it's not.

(In reply to comment #3) > we need Talos numbers on this change, maybe you could do a quick Tryserver run. I see you were specifically asking about this for the exclusive lock change. I'll push it to places in two parts so we know for sure.

Also, we need to close out our database connection at shutdown, which I've done locally.

http://hg.mozilla.org/projects/places/rev/dcd52d7004f0 http://hg.mozilla.org/projects/places/rev/8e82641697dc

Er hey, I should have also opened the browser. Landed a part 3 to fix an issue that cropped up on tinderbox: http://hg.mozilla.org/projects/places/rev/ac2c429e6273 I don't think mak needs to look at this and give it an official review, but comment if you feel otherwise!

http://hg.mozilla.org/mozilla-central/rev/dcd52d7004f0 http://hg.mozilla.org/mozilla-central/rev/8e82641697dc http://hg.mozilla.org/mozilla-central/rev/ac2c429e6273