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.

Attaching a patch that performs actual loading/migration of metadata asynchronously.

Any hope of a review?

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?

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.

Do you want me to review this patch then or wait?

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 on attachment 632687 [details] [diff] [review] Loading metadata asynchronously Yes, let's do that.

Here we go. As expected, this code is much nicer than the previous version.

Setting as blocker, as this bug lies on the way of ongoing Snappy work.

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?

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

(same patch, with a title)

Try: https://tbpl.mozilla.org/?tree=Try&rev=19f2fb07fd1d

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

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

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.

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.

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

@mak That's good to know, fixing this. It should really be added to the documentation, though.

Ok, with bug 823502, tests pass: https://tbpl.mozilla.org/?tree=Try&rev=278e1facec9b

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 :)

Do we have existing good test coverage here? https://hg.mozilla.org/integration/mozilla-inbound/rev/98dddd0da122

Normally, the tests were checked in as part of the previous bug.