Closed Bug 599978 Opened 9 years ago Closed 9 years ago

Asynchronous isVisited checks should use a read-only cloned connection

Categories

(Toolkit :: Places, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b9

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

(Whiteboard: [fixed-in-places])

Attachments

(2 files, 2 obsolete files)

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.
Attached patch v0.1 (obsolete) — Splinter Review
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...
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attached patch v1.0 (obsolete) — Splinter Review
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.
Attachment #481096 - Attachment is obsolete: true
Attachment #481225 - Flags: review?(mak77)
Whiteboard: [needs reivew mak]
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.
Attachment #481225 - Flags: review?(mak77) → review+
Whiteboard: [needs reivew mak] → [needs updated patch]
(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.
Attachment #481225 - Attachment is obsolete: true
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!
Depends on: 604130
Depends on: 604199
http://hg.mozilla.org/mozilla-central/rev/dcd52d7004f0
http://hg.mozilla.org/mozilla-central/rev/8e82641697dc
http://hg.mozilla.org/mozilla-central/rev/ac2c429e6273
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b9
You need to log in before you can comment on or make changes to this bug.