Closed Bug 988292 Opened 10 years ago Closed 10 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: rvitillo, Assigned: rvitillo)

References

(Blocks 1 open bug)

Details

(Keywords: main-thread-io)

Attachments

(1 file, 7 obsolete files)

{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.
Assignee: nobody → rvitillo
Any guesses on where this is coming from, Felipe?
Flags: needinfo?(felipc)
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.
Flags: needinfo?(felipc)
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.
Attachment #8398531 - Flags: review?(felipc)
Attachment #8398531 - Attachment is obsolete: true
Attachment #8399525 - Flags: review?(felipc)
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?
Attachment #8399525 - Flags: review?(felipc) → feedback+
Attachment #8399525 - Attachment is obsolete: true
Attachment #8399919 - Flags: review?(bmcbride)
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.
Attachment #8399919 - Flags: feedback+
Flags: needinfo?(felipc)
(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.
Attachment #8399919 - Attachment is obsolete: true
Attachment #8399919 - Flags: review?(bmcbride)
Attachment #8403976 - Flags: review?(irving)
(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.
Flags: needinfo?(felipc)
Patch updated to use the "new Promise(function(resolve, reject) {...})" style as discussed on IRC.
Attachment #8403976 - Attachment is obsolete: true
Attachment #8403976 - Flags: review?(irving)
Attachment #8404682 - Flags: review?(irving)
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;
}
Attachment #8404682 - Flags: review?(irving) → feedback+
Attachment #8404682 - Attachment is obsolete: true
Attachment #8404797 - Flags: review?(irving)
Attachment #8404797 - Flags: review?(irving) → review+
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.
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
I removed the FileUtils dependency and applied Yoric's suggestion.
Attachment #8404797 - Attachment is obsolete: true
Tests are still failing on Windows after removing FileUtils
https://tbpl.mozilla.org/?tree=Try&rev=29d404709278
Depends on: 900954
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.
Attachment #8405385 - Attachment is obsolete: true
Attachment #8414489 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ac59b6185d61
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.