Closed Bug 599973 Opened 9 years ago Closed 9 years ago

Don't use steps for async favicons

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: sdwilsh, Assigned: mak)

References

Details

(Whiteboard: [fixed-in-places])

Attachments

(2 files, 10 obsolete files)

78.09 KB, patch
Details | Diff | Splinter Review
10.52 KB, patch
Details | Diff | Splinter Review
Using steps for adding favicons seemed like a good idea at the time, but it has some major drawbacks:
1) Increased number of fsyncs, even if they don't happen on the main thread.
2) Lost atomicity of the work.  Things can happen in the middle of our operations, which can lead to weird bugs.

Due to (1), any main thread I/O that does happen can take much longer since the disk is being told to sync.  This is especially true if the same file has that going on, which is the case with places.

(2) just gives us more possibility for intermittent orange, and those things could happen to our users too.

Note that this is very similar to bug 599969.
Depends on: 553489
I'm starting having suspect favicons are hurting tp4 more now that visits addition does not use steps, thus I guess I'll drop all the stepper code and make favicons setting more similar to what you did in bug 599969, having all write statements in a single transaction in the final step.

But first of all I've sent a new changeset to tryserver to measure how much favicons are hurting after your visits patch.
I've also tried to check IO with xperf during tp4, I've seen something like 350 writes to places.sqlite-wal that is sadfaces, unfortunately I'm still trying to get symbols working for the places branch builds.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Talos said with disabled favicons we are gaining about 11% in Win7 (applied on top of your visit-without-step patch), thus I suspect we could gain some percentage with this stuff and is worth doing it.

WinXP measure does not make sense, indeed it is showing 27% increase (whut?), the interesting fact is that the more we fix stuff the more xp and 7 measures are similar, I suspect XP measure was broken till today, or maybe just ignoring some work on shutdown, I don't see really reasons why XP should be 30% faster than win7 on Tp4.
Attached patch wip v1.0 (obsolete) — Splinter Review
Just saving a checkpoint. I had to rewrite most of the code to get rid of steps, but this is easier to read and looks better for I/O.
I was also able to kill some hated (and sync!) favicon method like updateBookmarksRedirectFavicon that was called for each notified icon.

TODO:
- pass tests
- cache statements somewhere
- Check numbers on try
- Measure IO change
Passes Places tests locally. Luckily I already improved tests when working on async steps.
statements cache will go in its own part.
Attachment #487649 - Attachment is obsolete: true
Attachment #487689 - Attachment is obsolete: true
This is a bit fancy, but faviconService is not thread-safe, thus I can't use it to directly hold a StatementCache. I've created a PlacesStatementsCache that uses thread-safe AddRefing, wrapping around StatementCache. faviconService and each runnable have a strong pointer to it.
Shawn, could be you want to directy do something for this in the base class, or have better suggestions on who should own the cache. I thought about directly reusing History cache but that would be mixing up services a bit.

Btw, these 2 patches on top of patches in bug 599969 should allow to measure what we care about.
Can you attach part 2 with more context please?  It's hard to tell what is going on with that small amount of context.

I don't see why you can't use the favicon service to directly hold a statement cache though - if it's done like History, it won't matter (StatementCache is not refcounted).  As long as you pass off the statement cache pointer/reference (I think I'd go with a reference actually) to your runnable when you create it, you'll be fine.

Note: this also assumes you'll only ever access it on one thread.  If you want to use it on the main thread and off the main thread, you really just want to create two caches.
well, if I just pass a reference it's possible the cache dies before my runnables, you added a strong ref to History, but it can be released in any of the 2 threads, something that can't happen for favicons service
with more context.

I'm not sure context would help part 1, it is mostly a rewrite, I guess it's easier to review if you apply it and directly review asyncFaviconHelpers.cpp/.h
Attachment #487789 - Attachment is obsolete: true
Attachment #487869 - Flags: review?(sdwilsh)
Attachment #487785 - Flags: review?(sdwilsh)
tp4 does not seem so good though, the only measure that improves is Win7, all the others are regressing. This is depressing. It is like Tp4 was not measuring all work, while with these changes it does and the most we improve writes the most it regresses.
(In reply to comment #8)
> well, if I just pass a reference it's possible the cache dies before my
> runnables, you added a strong ref to History, but it can be released in any of
> the 2 threads, something that can't happen for favicons service
We have to notify on the calling thread right?  Even if we don't, we have NS_ProxyRelease which we can use to make the reference disappear on the thread we specify.  I'd rather go with this approach than subclassing the storage helper to make it threadsafe (and add refcounting).
Yes, I can release it when we notify on main-thread, but in some case we early return and don't notify, I guess NS_ProxyRelease is fine here too.
The current patch should be fine for measures though, thus I'll wait to fix part 2 till we have numbers, we figured out what to do with visits, and part 1 is reviewed.
(In reply to comment #12)
> Yes, I can release it when we notify on main-thread, but in some case we early
> return and don't notify, I guess NS_ProxyRelease is fine here too.
We could NS_ProxyRelease in the destructor then so it always gets called :)
Comment on attachment 487869 [details] [diff] [review]
487789: part 2: Use a statements cache. v1.0

part 2 is not yet ready for review since I need to address the favicon service main-thread release stuff for caching statements, but since part 2 is on top of part 1 it would bitrot fast.

part 1 (that does not include statements caching) is ready for review though.
Attachment #487869 - Flags: review?(sdwilsh)
Comment on attachment 487785 [details] [diff] [review]
part 1: kill steps, do writes in a single transaction. v1.1

>+/**
>+ * Fetches information on a page from the Places database.
>+ */
Full javadoc stuff please (missing @params)

>+nsresult
>+FetchPageInfo(mozIStorageConnection* aDBConn,
>+              PageData& _page)
I think we should start moving to making anything we pass a connection to take a reference to an nsCOMPtr.  We used pointers so that we didn't have to AddRef and Release, but we can accomplish that too by using nsCOMPtr<mozIStorageConnection>& as well (and it will be safer!).

>+{
>+  NS_PRECONDITION(_page.spec.Length(), "Must have a non-empty spec!");
This also shouldn't ever be called on the main thread, right?  Please assert that.

>+/**
>+  * Fetches information on a icon from the Places database.
>+ */
ditto on java-doc comments (You do this again elsewhere.  Please fix it everywhere)

>+nsresult
>+FetchIconInfo(mozIStorageConnection* aDBConn, IconData& _icon)
ditto on nsCOMPtr reference
nit: _icon on new line

>+  // If the page does not exist url = NULL will return NULL instead of 0,
>+  // since (1 = NULL) is NULL.  Thus the need for the IFNULL.
>+  nsCOMPtr<mozIStorageStatement> stmt;
>+  aDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+    "SELECT id, expiration, data, mime_type "
>+    "FROM moz_favicons WHERE url = :icon_url"
>+  ), getter_AddRefs(stmt));
>+  NS_ENSURE_STATE(stmt);
This comment doesn't seem to make any sense given the query.

>+AsyncFetchAndSetIconForPage::AsyncFetchAndSetIconForPage(
>+  IconData &aIcon
>+, PageData &aPage
nit: & after type, no space

>+, mozIStorageConnection* aDBConn
>+, nsIFaviconDataCallback* aCallback
>+)
nit: nsCOMPtr<mozIStorageConnection>& aDBConn.

>+  PageData page;
>+  rv = aPageURI->GetSpec(page.spec);
>+  NS_ENSURE_SUCCESS(rv, rv);
Move to first use

>+  (void)GetReversedHostname(aPageURI, page.revHost);
We should just do this on the background thread I think.  No reason to do it here (don't need it until the background thread as far as I can tell)

The above comments apply to other parts of this patch as well (missing javadoc mostly), but otherwise this looks good.  r=sdwilsh
Attachment #487785 - Flags: review?(sdwilsh) → review+
blocking2.0: --- → betaN+
using the reference to the COMPtr is fine and I'll do it, but I feel it also error-crash-prone, where you could create a scoped COMPtr and pass it to an enqueued event. A simple example (pseudo-code)

nsCOMPtr<connection> dbConn;
history->getconnection(getter_AddRefs(dbConn));
event = new event(dbConn);
NS_dispatchToMainThread(event);

since event gets a reference, as soon as it starts the COMPtr is garbage.
It is fine if I have a local hard ownership in mDBConn, but again if my event survives to the object that created it, it will have a garbage ptr.
Should I NS_ENSURE_STATE the connection before each usage?
and notice there is no way to tell if event is AddRefing or not without looking at its definition.
Attachment #487785 - Attachment is obsolete: true
hm, the situation is still a bit cumbersome. For Icons it's different than for visits, because it's possible I have to do main->async->main->async or main->async->async (the second main-thread stuff is the async network fetch that could happen or not). I need one strong reference to Favicons Service for the cache and one to the callback, thus I have to guarantee that these objects are always addrefed and released on the same thread, but I also have to pass them through all runnables.
(In reply to comment #16)
> Should I NS_ENSURE_STATE the connection before each usage?
Yes, just do this (although, I think it's unlikely that the pointer would be NULL ever, if we have the ownership right).
yeah indeed I'll need strong ownership on the favicons service, that has strong ownership on the connection, thus I don't need further checks.
Attachment #489792 - Attachment is obsolete: true
Attachment #487869 - Attachment is obsolete: true
Depends on: 611639
Asking a second pass review.
I've fixed thread-safety issues, and previous comments.
I've filed bug 611639 on the ProxyRelease issue, after confirming that a correct overload for RefPtrs solves the crash.
Attachment #489999 - Attachment is obsolete: true
Attachment #490075 - Flags: review?(sdwilsh)
this is luckily much easier now that part 1 handles thread-safety
Attachment #490005 - Attachment is obsolete: true
Attachment #490076 - Flags: review?(sdwilsh)
Blocks: 612281
Comment on attachment 490075 [details] [diff] [review]
part 1 (v1.3): kill steps, do writes in a single transaction.

>+  nsRefPtr<AsyncFetchAndSetIconForPage> event =
>+    new AsyncFetchAndSetIconForPage(icon, page, aDBConn, fs, callback);
>+  NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
We do not need to check the result of new anymore.  Just things that are malloc'd.

>+  if (!fetchIconFromNetwork) {
>+    // There is already a valid icon or we don't want to fetch a new one,
>+    // directly proceed with association.
>+    nsRefPtr<AsyncAssociateIconToPage> event =
>+        new AsyncAssociateIconToPage(mIcon, mPage, mDBConn, mFaviconSvc, mCallback);
>+    NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
ditto

>+  else {
>+    // Fetch the icon from network.  When done this will associate the
>+    // icon to the page and notify.
>+    nsRefPtr<AsyncFetchAndSetIconFromNetwork> event =
>+      new AsyncFetchAndSetIconFromNetwork(mIcon, mPage, mDBConn, mFaviconSvc, mCallback);
>+    NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
ditto

>+    // The rev_host can be null.
>+    if (mPage.revHost.IsEmpty()) {
>+      rv = stmt->BindNullByName(NS_LITERAL_CSTRING("rev_host"));
>+    } else {
>+      rv = stmt->BindStringByName(NS_LITERAL_CSTRING("rev_host"), mPage.revHost);
>+    }
nit: bracing doesn't conform to style.

Maybe we should make a helper for binding the RevHost?  This bit of code always seems to be used a lot.

>+  // Finally, dispatch an event to the main thread to notify observers.
>+  nsCOMPtr<nsIRunnable> event = new NotifyIconObservers(mIcon, mPage, mDBConn, mFaviconSvc, mCallback);
>+  NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
don't check new

>+      // This will be silent, so be sure to not pass in the current callback.
>+      nsCOMPtr<nsIFaviconDataCallback> nullCallback;
>+      nsRefPtr<AsyncAssociateIconToPage> event =
>+          new AsyncAssociateIconToPage(mIcon, bookmarkedPage, mDBConn, mFaviconSvc, nullCallback);
>+      NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
ditto

r=sdwilsh
Attachment #490075 - Flags: review?(sdwilsh) → review+
Comment on attachment 490076 [details] [diff] [review]
part 2 (v1.2): Use a statementCache.

>+++ b/toolkit/components/places/src/AsyncFaviconHelpers.cpp
> nsresult
>-FetchPageInfo(nsCOMPtr<mozIStorageConnection>& aDBConn,
>+FetchPageInfo(mozilla::storage::StatementCache<mozIStorageStatement>& aStmtCache,
>               PageData& _page)
At the very least, do a using namespace mozilla; at the top of this file so we can just do storage::StatementCache<mozIStorageStatement>& please.  I'd be fine with using namespace mozilla::storage too.

>@@ -934,16 +935,18 @@ nsFaviconService::FinalizeStatements() {
>     mDBRemoveAllFavicons,
>   };
> 
>   for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(stmts); i++) {
>     nsresult rv = nsNavHistory::FinalizeStatement(stmts[i]);
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
>+  mSyncStatements.FinalizeStatements();
We could potentially finalize statements that are still running on the background thread.  Additionally, we should only be touching mSyncStatements on the background thread here, right?  Dispatch this in a runnable please (and please file a bug if we do this wrong in History.cpp, which I think we do :/).

>+  /**
>+   * This cache should be used only for background thread statements.
>+   */
>+  mozilla::storage::StatementCache<mozIStorageStatement> mSyncStatements;
nit: add
@pre must be running on the background thread of mDBConn.

r=sdwilsh with these changes
Attachment #490076 - Flags: review?(sdwilsh) → review+
(In reply to comment #26)
> Maybe we should make a helper for binding the RevHost?  This bit of code always
> seems to be used a lot.

Maybe, we could even just add a BindRevHost to URIBinder, but I don't want to handle that here, we can probably do these nits later in the cycle when we'll remove old migration code and do other cleanups. I'll file a followup.
Fixed comments, I also found a thread-safety bug, I was not releasing mChannel on the right thread, I'm doing it now.
Attachment #490075 - Attachment is obsolete: true
addressed comments, filed bug 612455 for History.cpp statements finalization.
Attachment #490076 - Attachment is obsolete: true
Depends on: 612592
part1: http://hg.mozilla.org/mozilla-central/rev/999385f355a3
part2: http://hg.mozilla.org/mozilla-central/rev/a4bd5f0ad569
bustage fix: http://hg.mozilla.org/mozilla-central/rev/6617dcb0d48a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b9
Blocks: 497405
Depends on: 621274
Depends on: 621273
Depends on: 625798
Blocks: 554553
You need to log in before you can comment on or make changes to this bug.