599973 - Don't use steps for async favicons

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Shawn Wilsher :sdwilsh]

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.

Comment 1

[Marco Bonardo [:mak]]

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.

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Marco Bonardo [:mak]]

Attachment: wip v1.0

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

Comment 4

[Marco Bonardo [:mak]]

Attachment: part1: kill steps, do writes in a single transaction. v1.0

Passes Places tests locally. Luckily I already improved tests when working on async steps.
statements cache will go in its own part.

Comment 5

[Marco Bonardo [:mak]]

Attachment: part 1: kill steps, do writes in a single transaction. v1.1

Comment 6

[Marco Bonardo [:mak]]

Attachment: part 2: Use a statements cache. v1.0

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.

Comment 7

[Shawn Wilsher :sdwilsh]

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.

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[Marco Bonardo [:mak]]

Attachment: 487789: part 2: Use a statements cache. v1.0

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

Comment 10

[Marco Bonardo [:mak]]

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.

Comment 11

[Shawn Wilsher :sdwilsh]

(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).

Comment 12

[Marco Bonardo [:mak]]

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.

Comment 13

[Shawn Wilsher :sdwilsh]

(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 14

[Marco Bonardo [:mak]]

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.

Comment 15

[Shawn Wilsher :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

Comment 16

[Marco Bonardo [:mak]]

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?

Comment 17

[Marco Bonardo [:mak]]

and notice there is no way to tell if event is AddRefing or not without looking at its definition.

Comment 18

[Marco Bonardo [:mak]]

Attachment: part1: kill steps, do writes in a single transaction. v1.1

Comment 19

[Marco Bonardo [:mak]]

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.

Comment 20

[Shawn Wilsher :sdwilsh]

(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).

Comment 21

[Marco Bonardo [:mak]]

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.

Comment 22

[Marco Bonardo [:mak]]

Attachment: part 1 (v1.2): kill steps, do writes in a single transaction.

Comment 23

[Marco Bonardo [:mak]]

Attachment: part 2 (v1.1): Use a statementCache.

Comment 24

[Marco Bonardo [:mak]]

Attachment: part 1 (v1.3): kill steps, do writes in a single transaction.

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.

Comment 25

[Marco Bonardo [:mak]]

Attachment: part 2 (v1.2): Use a statementCache.

this is luckily much easier now that part 1 handles thread-safety

Comment 26

[Shawn Wilsher :sdwilsh]

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

Comment 27

[Shawn Wilsher :sdwilsh]

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

Comment 28

[Marco Bonardo [:mak]]

(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.

Comment 29

[Marco Bonardo [:mak]]

Attachment: part 1 (v1.4): kill steps, do writes in a single transaction.

Fixed comments, I also found a thread-safety bug, I was not releasing mChannel on the right thread, I'm doing it now.

Comment 30

[Marco Bonardo [:mak]]

Attachment: part 2 (v1.3): Use a statementCache.

addressed comments, filed bug 612455 for History.cpp statements finalization.

Comment 31

[Marco Bonardo [:mak]]

part 1: http://hg.mozilla.org/projects/places/rev/999385f355a3
part 2: http://hg.mozilla.org/projects/places/rev/a4bd5f0ad569

Comment 32

[Marco Bonardo [:mak]]

pushed a bustage fix: http://hg.mozilla.org/projects/places/rev/6617dcb0d48a

Comment 33

[Marco Bonardo [:mak]]

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