988292 - Avoid main-thread IO for {profile}\addons.json

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Roberto Agostino Vitillo (:rvitillo)]

{profile}\addons.json is a main-thread IO offender in terms of number of Telemetry submissions where it appears in (~ 25%) after filtering out IO operations during startup and shutdown.

Comment 1

[Vladan Djeric (:vladan)]

Any guesses on where this is coming from, Felipe?

Comment 2

[Vladan Djeric (:vladan)]

Is this bug 968829?

Comment 3

[Roberto Agostino Vitillo (:rvitillo)]

Vladan, see http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/AddonRepository.jsm#1545

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Yeah, this was intentionally left as a follow-up issue on bug 853389, because this would require API changes and we wanted to have solid evidence first that the all the work from bug 853389 didn't cause any regressions. It can be done now.

Comment 5

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Avoid main-thread IO for {profile}\addons.json, v1

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

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8398531 [details] [diff] [review]
Avoid main-thread IO for {profile}\addons.json, v1

Review of attachment 8398531 [details] [diff] [review]:
-----------------------------------------------------------------

Since openConnection is becoming async, we won't need to have the special case handling for the DB migration. See how retrievedStoredData is contrived right now, but with the async'ification, openConnection should resolve only after migration has finished, and then retrieveStoredData wouldn't need to have two different paths for migrationInProgress or not.

Comment 7

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Avoid main-thread IO for {profile}\addons.json, v2

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8399525 [details] [diff] [review]
Avoid main-thread IO for {profile}\addons.json, v2

Review of attachment 8399525 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but Blair should be the reviewer for it.

I just realized that these new functions would be good candidates for Task.async (http://mykzilla.blogspot.fr/2014/03/simplify-asynchronous-method.html), but it's up to you if you want to use this new helper

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm
@@ +1592,3 @@
>  
> +           Services.prefs.setIntPref(PREF_GETADDONS_DB_SCHEMA, DB_SCHEMA);
> +           return deferred.promise;

if a Task function returns a promise, is that task only resolved when this promise is resolved?

Comment 9

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Avoid main-thread IO for {profile}\addons.json, v3

Comment 10

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8399919 [details] [diff] [review]
Avoid main-thread IO for {profile}\addons.json, v3

Review of attachment 8399919 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm
@@ +1540,3 @@
>     */
>    openConnection: function() {
> +    if (!this.connectionPromise) {

It might be because I wrote it, but I prefer the style I used in XPIProvider.jsm, where the async load-the-database function returns a promise that resolves to the database itself. That leaves all the async API implementations looking like:

this.getDatabase().then(database => {implementation})

or, if you prefer Task style,

let database = yield this.getDatabase();
... implementation

@@ +1580,3 @@
>  
> +         if (dbSchema < DB_MIN_JSON_SCHEMA) {
> +           let deferred = Promise.defer();

Question for Yoric/Paolo: do we prefer the "new Promise(function(resolve, reject) {...}) way of making promises?

@@ +1592,2 @@
>  
> +           Services.prefs.setIntPref(PREF_GETADDONS_DB_SCHEMA, DB_SCHEMA);

I realize that this is the behaviour of the existing code, but do we really want to update the schema version preference here *before* the async database migration completes? What if it fails?

@@ +1607,5 @@
> +       // insertAddons to avoid the write to disk which would
> +       // be a waste since this is the data that was just read.
> +       for (let addon of inputDB.addons) {
> +         this._insertAddon(addon);
> +       }

This could be converted to your new this._insertAddons(inputDB.addons);

@@ +1713,2 @@
>  
> +    executeSoon(function() aCallback(result));

The 'yield this.openConnection()' at the top of this function unwinds the stack, so we don't really need the 'executeSoon()' wrapper on the callback here any more.

There's only one caller, inside this file, so we could also just change the API to return a promise of the 'result' object.

Again, this is how the existing code behaves, but I'm not happy about making a temp copy of the addons database every time this function is called - XPIProvider loops calling getCachedAddonByID(), which calls this, once for each add-on so we're throwing away a lot of work.

Comment 11

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to :Irving Reid from comment #10)

> @@ +1607,5 @@
> > +       // insertAddons to avoid the write to disk which would
> > +       // be a waste since this is the data that was just read.
> > +       for (let addon of inputDB.addons) {
> > +         this._insertAddon(addon);
> > +       }
> 
> This could be converted to your new this._insertAddons(inputDB.addons);

As stated in the comment, _insertAddon is called manually instead of insertAddons to avoid the write to disk.

Comment 12

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Avoid main-thread IO for {profile}\addons.json, v4

Comment 13

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Irving Reid from comment #10)
> Again, this is how the existing code behaves, but I'm not happy about making
> a temp copy of the addons database every time this function is called -
> XPIProvider loops calling getCachedAddonByID(), which calls this, once for
> each add-on so we're throwing away a lot of work.

getCachedAddonByID caches itself the result, and only calls retrieveStoredData once or when the cache has been invalidated. So there's no big problem in doing that as it doesn't iterate through the DB more than once unnecessarily.

Comment 14

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Avoid main-thread IO for {profile}\addons.json, v5

Patch updated to use the "new Promise(function(resolve, reject) {...})" style as discussed on IRC.

Comment 15

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8404682 [details] [diff] [review]
Avoid main-thread IO for {profile}\addons.json, v5

Review of attachment 8404682 [details] [diff] [review]:
-----------------------------------------------------------------

One more simplification and it's good. I'll file a follow up for some of the pre-existing conditions.

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm
@@ +1582,5 @@
>  
> +         if (dbSchema < DB_MIN_JSON_SCHEMA) {
> +           return new Promise((resolve, reject) => {
> +             AddonRepository_SQLiteMigrator.migrate((results) => {
> +               resolve(Task.spawn(function* (){

We're already inside a Task.spawn(), at the outer layer of openConnection(), so we shouldn't need another one here; I think this can be simplified to:

if (dbSchema...) {
  let results = yield new Promise((resolve, reject) => AR_SQLMigrator.migrate(resolve));
  if (results.length) {
    yield this._insertAddons(results);
  }
  ..setIntPref...
  return this.DB;
}

Comment 16

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Avoid main-thread IO for {profile}\addons.json, v6

Comment 17

[Roberto Agostino Vitillo (:rvitillo)]

There are some failures on Windows: https://tbpl.mozilla.org/?tree=Try&rev=2957d1d1a6db

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8404797 [details] [diff] [review]
Avoid main-thread IO for {profile}\addons.json, v6

Review of attachment 8404797 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm
@@ +1549,5 @@
>  
> +       try {
> +         let array = yield OS.File.read(this.jsonFile.path);
> +         let decoder = new TextDecoder();
> +         let data = decoder.decode(array);

You should replace all three lines with
  let data = yield OS.File.read(this.jsonFile.path, { encoding: "utf-8"})
This will be at least as fast for all files and much faster for large files.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

At a quick glance, the Windows errors remind me of race conditions caused by the use of both OS.File and nsIFile in and around XPIProvider.jsm

Comment 20

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Avoid main-thread IO for {profile}\addons.json, v7

I removed the FileUtils dependency and applied Yoric's suggestion.

Comment 21

[Roberto Agostino Vitillo (:rvitillo)]

Tests are still failing on Windows after removing FileUtils
https://tbpl.mozilla.org/?tree=Try&rev=29d404709278

Comment 22

[:Irving Reid (No longer working on Firefox)]

Attachment: Avoid main-thread IO for {profile}\addons.json, conflicts resolved

r+ for Roberto's v7 patch, but I needed to update it for landing because it modified an adjacent line to the patch for bug 900954 so it didn't apply cleanly.

Comment 23

[:Irving Reid (No longer working on Firefox)]

https://hg.mozilla.org/integration/fx-team/rev/ac59b6185d61

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac59b6185d61