Last Comment Bug 760036 - nsIBrowserSearchService should load metadata asynchronously
: nsIBrowserSearchService should load metadata asynchronously
Status: RESOLVED FIXED
[Snappy]
: main-thread-io
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: unspecified
: All All
: -- blocker (vote)
: Firefox 20
Assigned To: David Teller [:Yoric] (please use "needinfo")
:
:
Mentors:
Depends on: 722332 817435 823502 857588 866254
Blocks: 706523
  Show dependency treegraph
 
Reported: 2012-05-31 03:17 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2013-05-03 16:10 PDT (History)
9 users (show)
gavin.sharp: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Dividing the initialization process in steps (17.35 KB, patch)
2012-06-13 02:06 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Loading metadata asynchronously (22.47 KB, patch)
2012-06-13 07:33 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Metadata management using OMT I/O and synchronous fallbac (27.46 KB, patch)
2012-11-19 12:41 PST, David Teller [:Yoric] (please use "needinfo")
enndeakin: review+
Details | Diff | Splinter Review
Metadata management using OMT I/O and synchronous fallback, v2 (25.64 KB, patch)
2012-12-01 05:53 PST, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
Metadata management using OMT I/O and synchronous fallback, v2 (25.63 KB, patch)
2012-12-01 23:38 PST, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2012-05-31 03:17:37 PDT
Bug 722332 makes nsIBrowserSearchService API async, with a synchronous fallback. At the moment, the implementation is mostly synchronous. This bug is about rewriting the initialization of |engineMetadataService| to introduce an asynchronous path.
Comment 1 David Teller [:Yoric] (please use "needinfo") 2012-06-13 02:06:12 PDT
Created attachment 632607 [details] [diff] [review]
Dividing the initialization process in steps
Comment 2 David Teller [:Yoric] (please use "needinfo") 2012-06-13 07:33:13 PDT
Created attachment 632687 [details] [diff] [review]
Loading metadata asynchronously

Attaching a patch that performs actual loading/migration of metadata asynchronously.
Comment 3 David Teller [:Yoric] (please use "needinfo") 2012-09-21 05:54:56 PDT
Any hope of a review?
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-14 12:31:55 PST
Comment on attachment 632687 [details] [diff] [review]
Loading metadata asynchronously

Neil's going to look into this one.

David, IIRC you mentioned potentially having an updated version of this patch?
Comment 5 David Teller [:Yoric] (please use "needinfo") 2012-11-14 12:37:01 PST
I don't have one yet, but I am willing to write one, if necessary.
This patch was written before Promises, OS.File and Tasks.jsm landed on m-c, so they could all be used potentially to simplify the async path.
Comment 6 Neil Deakin (away until Oct 3) 2012-11-15 09:59:28 PST
Do you want me to review this patch then or wait?
Comment 7 David Teller [:Yoric] (please use "needinfo") 2012-11-15 14:08:02 PST
If you are willing to review a patch that makes use of Promise, OS.File and Task.jsm, I would prefer rewriting it first.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-16 10:52:09 PST
Comment on attachment 632687 [details] [diff] [review]
Loading metadata asynchronously

Yes, let's do that.
Comment 9 David Teller [:Yoric] (please use "needinfo") 2012-11-19 12:41:29 PST
Created attachment 683266 [details] [diff] [review]
Metadata management using OMT I/O and synchronous fallbac

Here we go. As expected, this code is much nicer than the previous version.
Comment 10 David Teller [:Yoric] (please use "needinfo") 2012-11-28 02:28:21 PST
Setting as blocker, as this bug lies on the way of ongoing Snappy work.
Comment 11 Neil Deakin (away until Oct 3) 2012-11-28 07:15:53 PST
Comment on attachment 683266 [details] [diff] [review]
Metadata management using OMT I/O and synchronous fallbac

There are some places you add blank lines but I don't think it is necessary. 

The indenting you use on switch statements (in init() and elsewhere) makes the code a bit hard to read where each block ends. Instead, you the same style as the file. 

+  // Used by |_ensureInitialized| as a fallback if initialization is not
+  // complete. In this implementation, it is also used by |init|, although
+  // this will not last forever.

- Can you clarify the last part of this comment?

-      LOG("_buildCache: Writing to cache file.");

Why is this removed?
Comment 12 David Teller [:Yoric] (please use "needinfo") 2012-12-01 05:10:05 PST
(In reply to Neil Deakin from comment #11)
> Comment on attachment 683266 [details] [diff] [review]
> Metadata management using OMT I/O and synchronous fallbac
> 
> There are some places you add blank lines but I don't think it is necessary.

Ok. 

> The indenting you use on switch statements (in init() and elsewhere) makes
> the code a bit hard to read where each block ends. Instead, you the same
> style as the file. 

Ok.

> +  // Used by |_ensureInitialized| as a fallback if initialization is not
> +  // complete. In this implementation, it is also used by |init|, although
> +  // this will not last forever.
> 
> - Can you clarify the last part of this comment?

Bug 706523 et al will require fully reimplementing |init|, without |syncInit|.

> 
> -      LOG("_buildCache: Writing to cache file.");
> 
> Why is this removed?

Overzealous pre-review cleanup :)

Thanks for the review!
Comment 13 David Teller [:Yoric] (please use "needinfo") 2012-12-01 05:53:41 PST
Created attachment 687396 [details] [diff] [review]
Metadata management using OMT I/O and synchronous fallback, v2
Comment 14 David Teller [:Yoric] (please use "needinfo") 2012-12-01 23:38:56 PST
Created attachment 687496 [details] [diff] [review]
Metadata management using OMT I/O and synchronous fallback, v2

(same patch, with a title)
Comment 15 David Teller [:Yoric] (please use "needinfo") 2012-12-01 23:55:15 PST
Try: https://tbpl.mozilla.org/?tree=Try&rev=19f2fb07fd1d
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-12-02 11:08:19 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd72ae8aaff

Though looking at your Try run, I'm a bit nervous about the fact that your robocop runs all failed for the same reason. We'll see if it sticks.
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-12-02 11:27:52 PST
Comment on attachment 687496 [details] [diff] [review]
Metadata management using OMT I/O and synchronous fallback, v2

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+   _asyncMigrateOldDB: function SRCH_SVC_EMS_asyncMigrate() {

>+       const statement = sqliteDb.createStatement("SELECT * from engine_data");

Looks like this should be createAsyncStatement, but I'm not sure whether that matters much in practice.

I pushed a small followup for whitespace nits: https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4151d4d695

Awesome that this landed, thanks for your work on this David.
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-12-02 13:06:44 PST
Backed out for the aforementioned robocop failures and the Linux64 mochitest-bc failure (also showing on the Try push).
https://hg.mozilla.org/integration/mozilla-inbound/rev/3656ad584ae7
Comment 19 Ryan VanderMeulen [:RyanVM] 2012-12-02 17:28:21 PST
I think this was also the cause of bug 817435 (and PGO-only!!!). At least it went away on the next PGO run after the backout.
Comment 20 David Teller [:Yoric] (please use "needinfo") 2012-12-03 00:22:11 PST
It looks like the robocop failures (bug 769524) are actually due to a much larger issue somewhere in Fennec, so we will have to either wait until that issue is solved (preferably) or hack around that issue (by deactivating async I/O in the search service for Android) temporarily. For the moment, waiting for bug 769524.
Comment 21 Marco Bonardo [::mak] 2012-12-03 02:04:07 PST
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #17)
> Looks like this should be createAsyncStatement, but I'm not sure whether
> that matters much in practice.

It matters a lot, createStatement can be executed on both threads so it has to use a mutex, then when you execute it asynchronously it can create contention. We should never use createStatement when async.
Comment 22 David Teller [:Yoric] (please use "needinfo") 2012-12-03 02:06:27 PST
@mak

That's good to know, fixing this. It should really be added to the documentation, though.
Comment 23 David Teller [:Yoric] (please use "needinfo") 2012-12-20 06:48:34 PST
Ok, with bug 823502, tests pass: https://tbpl.mozilla.org/?tree=Try&rev=278e1facec9b
Comment 24 David Teller [:Yoric] (please use "needinfo") 2012-12-21 08:25:33 PST
As I will be away for two weeks, I will try to avoid the "Land Patch, Go Home", so I am not going to mark this checkin-needed just yet. If someone else wants to take the chance while I am away, be my guest :)
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-12-21 10:03:14 PST
Do we have existing good test coverage here?

https://hg.mozilla.org/integration/mozilla-inbound/rev/98dddd0da122
Comment 26 David Teller [:Yoric] (please use "needinfo") 2012-12-21 11:07:49 PST
Normally, the tests were checked in as part of the previous bug.
Comment 27 :Ms2ger (⌚ UTC+1/+2) 2012-12-22 06:45:09 PST
https://hg.mozilla.org/mozilla-central/rev/98dddd0da122

Note You need to log in before you can comment on or make changes to this bug.