nsIBrowserSearchService should load metadata asynchronously

RESOLVED FIXED in Firefox 20

Status

()

Firefox
Search
--
blocker
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

({main-thread-io})

unspecified
Firefox 20
main-thread-io
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(1 attachment, 4 obsolete attachments)

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.
Created attachment 632607 [details] [diff] [review]
Dividing the initialization process in steps
Assignee: nobody → dteller
Created attachment 632687 [details] [diff] [review]
Loading metadata asynchronously

Attaching a patch that performs actual loading/migration of metadata asynchronously.
Attachment #632607 - Attachment is obsolete: true
Attachment #632687 - Flags: review?(gavin.sharp)
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?
Attachment #632687 - Flags: review?(gavin.sharp) → review?(enndeakin)
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.
Attachment #632687 - Attachment is obsolete: true
Attachment #632687 - Flags: review?(enndeakin)
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.
Attachment #683266 - Flags: review?(enndeakin)
Blocks: 706523
Setting as blocker, as this bug lies on the way of ongoing Snappy work.
Severity: enhancement → blocker
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?
Attachment #683266 - Flags: review?(enndeakin) → review+
(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!
Created attachment 687396 [details] [diff] [review]
Metadata management using OMT I/O and synchronous fallback, v2
Attachment #683266 - Attachment is obsolete: true
Attachment #687396 - Flags: review+
Created attachment 687496 [details] [diff] [review]
Metadata management using OMT I/O and synchronous fallback, v2

(same patch, with a title)
Attachment #687396 - Attachment is obsolete: true
Attachment #687496 - Flags: review+
Try: https://tbpl.mozilla.org/?tree=Try&rev=19f2fb07fd1d
Keywords: checkin-needed
Whiteboard: [Snappy]
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.
Keywords: checkin-needed
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.
Depends on: 769524
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.
Depends on: 817435
Depends on: 823502
Ok, with bug 823502, tests pass: https://tbpl.mozilla.org/?tree=Try&rev=278e1facec9b
No longer depends on: 769524
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
Flags: in-testsuite?
Target Milestone: --- → Firefox 20
Normally, the tests were checked in as part of the previous bug.
https://hg.mozilla.org/mozilla-central/rev/98dddd0da122
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 825232
No longer depends on: 825232

Updated

4 years ago
Depends on: 857588

Updated

4 years ago
Depends on: 866254
You need to log in before you can comment on or make changes to this bug.