Closed Bug 862127 Opened 11 years ago Closed 6 years ago

profile migrators do all sorts of main thread I/O

Categories

(Firefox :: Migration, defect)

Product:
Firefox
Component:
Migration
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: Gavin, Assigned: alexical)

References

(Blocks 3 open bugs)

Details

(Keywords: main-thread-io, perf, Whiteboard: [async])

Attachments

(4 files)

Bug 862127 - Make migration interfaces more async
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 862127 - Convert migration.js to use async migration interfaces
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 862127 - Remove all sync IO from Chrome migration
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 862127 - Fix migrator tests to use async interfaces
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
Mostly a tracking bug - fixing this may require adjusting the APIs. I suppose bug 718280 could help. Since they're all JS now, it should be relatively easy to switch to OS.File, assuming an async API?
I'd like to recommend that we create automated tests (bug 700898 with a WIP in bug 706017) before making significant changes to migrator code.
For file access, sure, we could move to OS.File.
other parts may be synchronous just cause there's no async API usable, in case of bookmarks for example.
Btw, did this come out of measurements or from reports of migrators being slow?
It came out of me doing MXR searches for main thread I/O :)

I was mostly referring to the I/O from reading/copying from the "other" profiles, as opposed to our APIs for saving the migrated data.
Assignee: nobody → mano
Already working on this silently for some time now, so taking.
Whiteboard: [async]
Keywords: perf
I think you worked on migrators not too long ago, do you know if this bug is still relevant?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Florian Quèze [:florian] from comment #5)
> I think you worked on migrators not too long ago, do you know if this bug is
> still relevant?

Yes, definitely. See e.g. this query: https://dxr.mozilla.org/mozilla-central/search?q=netutil%20path%3Amigration&=mozilla-central . Really, the migration code needs rewriting to use async functions and await() and async OMT IO.

However... this is only used when migrating data from other browsers (or importing/exporting JSON/HTML bookmark files), which most users don't do (even when we show them that crappy wizard that suggests they do on first run). So not sure how much of a win it'd be to fix this.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #6)
> (In reply to Florian Quèze [:florian] from comment #5)
> > I think you worked on migrators not too long ago, do you know if this bug is
> > still relevant?
> 
> Yes, definitely. See e.g. this query:
> https://dxr.mozilla.org/mozilla-central/
> search?q=netutil%20path%3Amigration&=mozilla-central . Really, the migration
> code needs rewriting to use async functions and await() and async OMT IO.
> 
> However... this is only used when migrating data from other browsers (or
> importing/exporting JSON/HTML bookmark files), which most users don't do
> (even when we show them that crappy wizard that suggests they do on first
> run). So not sure how much of a win it'd be to fix this.

Also, I believe Doug is working on this stuff (bug 1421703), so he might be interested in this bug. :-)
Yeah, all of this is on the table for the purposes of opening up automigration (or similar techniques that we'd want to try that don't use a wizard). However the main thread IO for reading from the other browsers is, as far as I can observe, not the cause of most of our migration jank. Right now we still have a lot of work to do to reduce the main thread time spent processing all the data we bring in with that IO (notifying observers, encrypting logins, etc). But after that the main thread IO is probably next on the chopping block.
(In reply to Doug Thayer [:dthayer] from comment #8)

> However the main thread IO for reading from the other browsers is,
> as far as I can observe, not the cause of most of our migration jank.

I wonder what you are observing to reach this conclusion. Main thread IO causes pauses for unbound amounts of time, sometimes minutes. Looking at a profile captured on a fast developer computer with an SSD never shows this.
(In reply to Florian Quèze [:florian] from comment #9)
> I wonder what you are observing to reach this conclusion. Main thread IO
> causes pauses for unbound amounts of time, sometimes minutes. Looking at a
> profile captured on a fast developer computer with an SSD never shows this.

Sorry, I hadn't looked closely enough at this. We're focused on Chrome imports right now, and all that remains as far as I can see of main thread IO for that is for the sourceHomePageURL (I'm not sure this is even relevant for Chrome anymore? This doesn't seem to map to any setting I can find).

The other browsers still have quite a bit of main thread IO left over and you're right that that should be a priority if we want to get automigration off the ground for these browsers.
(In reply to Doug Thayer [:dthayer] from comment #10)
> (In reply to Florian Quèze [:florian] from comment #9)
> > I wonder what you are observing to reach this conclusion. Main thread IO
> > causes pauses for unbound amounts of time, sometimes minutes. Looking at a
> > profile captured on a fast developer computer with an SSD never shows this.
> 
> Sorry, I hadn't looked closely enough at this. We're focused on Chrome
> imports right now, and all that remains as far as I can see of main thread
> IO for that is for the sourceHomePageURL (I'm not sure this is even relevant
> for Chrome anymore? This doesn't seem to map to any setting I can find).

From a quick look, I'm fairly sure we still use main-thread IO for reading the list of profiles (which we generally need to do before (auto)import), and then a bunch of nsIFile.exists() and isReadable() calls (which are technically main-thread IO, I think?) in getResources() and the things it calls (to determine if Chrome has any importable resources), right?

As for sourceHomePageURL, I think it's offered in this pane of the wizard: https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/content/migration.js#274 . It's possible it's dead for Chrome if the import is broken for some reason, I'm not sure without trying.

Really, we should change getResources(), the `migrate()` function on it, and `get sourceProfiles()` to be async/await-based, which will then mean we can pretty much drop-in-replace some of it with OS.File, which will be relatively straightforward. As a nice bonus, doing that will help with de-XPCOM-taminating the code.

But as you rightly point out, whether this needs to be prioritized right now depends on the impact it's having. I don't know if, in the real (non-developer-machine-with-SSD) world, file.exists() is cheap enough that it's reasonably fast, or not.
(In reply to :Gijs from comment #11)

> I don't know if, in the real
> (non-developer-machine-with-SSD) world, file.exists() is cheap enough that
> it's reasonably fast, or not.

nsIFile.exists() uses stat, and it's main thread I/O, similar to a blocking read call in terms of cost: it's cheap most of the time and every once in a while blocks for more than 100ms (and seconds when extremely unlucky).
Also, there's a common bad pattern that does:
  if (file.exists())
    file.read();

This does main thread I/O twice, when:
  try {
    file.read();
  } catch(e) {}
would produce the same result for about half the cost.

But I haven't looked at the import code to check if this is the reason why .exist is being used.
(In reply to :Gijs from comment #11)
> > Sorry, I hadn't looked closely enough at this. We're focused on Chrome
> > imports right now, and all that remains as far as I can see of main thread
> > IO for that is for the sourceHomePageURL (I'm not sure this is even relevant
> > for Chrome anymore? This doesn't seem to map to any setting I can find).

IIRC in the past we discussed about completely removing the homepage migration, because in a lot of cases the homepage stays uncustomized and is very specific for the source browser and we feel like our homepage (even more with Activity Stream) can be more useful for the average user (provided any user can still customize it).
Assignee: asaf → nobody
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Comment on attachment 8942330 [details]
Bug 862127 - Make migration interfaces more async

https://reviewboard.mozilla.org/r/212592/#review218614
Attachment #8942330 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8942331 [details]
Bug 862127 - Convert migration.js to use async migration interfaces

https://reviewboard.mozilla.org/r/212594/#review218616

::: browser/components/migration/content/migration.js:46
(Diff revision 1)
>        this._source = args[1];
>        this._migrator = args[2] instanceof kIMig ? args[2] : null;
>        this._autoMigrate = args[3].QueryInterface(kIPStartup);
>        this._skipImportSourcePage = args[4];
>        if (this._migrator && args[5]) {
> -        let sourceProfiles = this._migrator.sourceProfiles;
> +        let sourceProfiles = MigrationUtils.spinResolve(this._migrator.getSourceProfiles());

I'm pretty nervous about this, mostly because I think in theory this can race with people clicking e.g. the forward/next button, and I bet things go pearshaped pretty quickly in that case (ie if we try to move forward without the first on`<whatever>` having run to completion.

Can you check what happens if you make getSourceProfiles() never resolve, and if it's possible to move forward or if the migrator stays stuck on the previous page or... something?
Attachment #8942331 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8942330 [details]
Bug 862127 - Make migration interfaces more async

https://reviewboard.mozilla.org/r/212592/#review218618

::: browser/components/migration/nsIBrowserProfileMigrator.idl:58
(Diff revision 1)
> -   * Whether or not there is any data that can be imported from this
> +   * Get whether or not there is any data that can be imported from this
>     * browser (i.e. whether or not it is installed, and there exists
>     * a user profile)
> +   * @return a promise that resolves with a boolean.
>     */
> -  readonly attribute boolean          sourceExists;
> +  jsval getSourceExists();

Nit: 'getFooExists' is kinda awkward. Maybe `isSourceAvailable` ?
Comment on attachment 8942332 [details]
Bug 862127 - Remove all sync IO from Chrome migration

https://reviewboard.mozilla.org/r/212596/#review218622

r=me with the below resolved one way or another (esp. the homepage URL thing)

::: browser/components/migration/ChromeProfileMigrator.js:137
(Diff revision 1)
>  ChromeProfileMigrator.prototype.getLastUsedDate =
> -  function Chrome_getLastUsedDate() {
> -    let datePromises = this.sourceProfiles.map(profile => {
> -      let basePath = OS.Path.join(this._chromeUserDataFolder.path, profile.id);
> -      let fileDatePromises = ["Bookmarks", "History", "Cookies"].map(leafName => {
> +  async function Chrome_getLastUsedDate() {
> +    let sourceProfiles = await this.getSourceProfiles();
> +    let chromeUserDataPath = await this._getChromeUserDataPathIfExists();
> +    if (!chromeUserDataPath) {
> +      throw new Error("Can't get last used date without User Data folder.");

Maybe just return `new Date(0)` ? Looks like this can't currently throw, so I'm a bit hesitant to make it so.

::: browser/components/migration/ChromeProfileMigrator.js
(Diff revision 1)
>        return resources && resources.length > 0;
> -    }, this);
> +    }, this).map(({profile}) => profile);
>      return this.__sourceProfiles;
>    };
>  
> -Object.defineProperty(ChromeProfileMigrator.prototype, "sourceHomePageURL", {

I know that it's annoying to have to update this, too, but I think it'd be better. You can presumably use the same pattern you've used for the other things in the first set of patches.

Alternatively, please talk to UX and/or Product about removing this. Michael Verdi and/or Peter Dolanjski are probably the right people to talk to. I agree with Marco that this is probably low-value, but we shouldn't just remove it in a tech/perf/cleanup patch without talking to folks.

::: browser/components/migration/ChromeProfileMigrator.js:327
(Diff revision 1)
> -  cookiesFile.append("Cookies");
> +  if (!(await OS.File.exists(cookiesPath)))
> -  if (!cookiesFile.exists())

Can you file a followup to make these and any other things that follow the pattern:

```js
if (foo.exists()) {
  // read foo
}
```

or 

```js
if (!foo.exists()) {
  return null
}
// read foo
```

just read `foo` and fail silently if the resulting error indicates the file doesn't exist?
Attachment #8942332 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8942333 [details]
Bug 862127 - Fix migrator tests to use async interfaces

https://reviewboard.mozilla.org/r/212598/#review218628

::: browser/components/migration/tests/unit/test_ChromeMigrationUtils_path.js
(Diff revision 1)
>      expectedPath = OS.Path.join(getRootPath(), ".config", "google-chrome");
>    }
>    Assert.equal(chromeUserDataPath, expectedPath, "Should get the path of Chrome user data directory.");
>  });
> -
> -add_task(async function test_getExtensionPath_function() {

Out of curiosity, why remove this?
Attachment #8942333 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8942331 [details]
Bug 862127 - Convert migration.js to use async migration interfaces

https://reviewboard.mozilla.org/r/212594/#review218616

> I'm pretty nervous about this, mostly because I think in theory this can race with people clicking e.g. the forward/next button, and I bet things go pearshaped pretty quickly in that case (ie if we try to move forward without the first on`<whatever>` having run to completion.
> 
> Can you check what happens if you make getSourceProfiles() never resolve, and if it's possible to move forward or if the migrator stays stuck on the previous page or... something?

It does allow the onpageadvanced and onpagerewound events to fire. What about something like this to disable those buttons while we're waiting for the promise to resolve?

```
  spinResolve(promise) {
    let canAdvance = this._wiz.canAdvance;
    let canRewind = this._wiz.canRewind;
    let canCancel = this._canCancel;
    this._wiz.canAdvance = false;
    this._wiz.canRewind = false;
    this._canCancel = false;
    let result = MigrationUtils.spinResolve(promise);
    this._wiz.canAdvance = canAdvance;
    this._wiz.canRewind = canRewind;
    this._canCancel = canCancel;
    return result;
  },

  onWizardCancel() {
    return this._canCancel;
  },
```
(In reply to Doug Thayer [:dthayer] from comment #24)
> Comment on attachment 8942331 [details]
> Bug 862127 - Convert migration.js to use async migration interfaces
> 
> https://reviewboard.mozilla.org/r/212594/#review218616
> 
> > I'm pretty nervous about this, mostly because I think in theory this can race with people clicking e.g. the forward/next button, and I bet things go pearshaped pretty quickly in that case (ie if we try to move forward without the first on`<whatever>` having run to completion.
> > 
> > Can you check what happens if you make getSourceProfiles() never resolve, and if it's possible to move forward or if the migrator stays stuck on the previous page or... something?
> 
> It does allow the onpageadvanced and onpagerewound events to fire. What
> about something like this to disable those buttons while we're waiting for
> the promise to resolve?

That seems OK to me, yeah.
> ```
>   onWizardCancel() {
>     return this._canCancel;
>   },
> ```

The only thing I wonder is if it's worth allowing cancel, but perhaps we can just leave that for a followup and play it safe for now.
(In reply to :Gijs from comment #25)
> The only thing I wonder is if it's worth allowing cancel, but perhaps we can
> just leave that for a followup and play it safe for now.

Yeah - I specifically added the cancel case in because canceling successfully closes the wizard, but the rest of Firefox is unresponsive until the promise resolves. We could have cancel force a stop on the spinEventLoopUntil call, though.
Comment on attachment 8942333 [details]
Bug 862127 - Fix migrator tests to use async interfaces

https://reviewboard.mozilla.org/r/212598/#review218628

> Out of curiosity, why remove this?

Woops! I had removed the extensions stuff since it wasn't used, before I realized that it was added in order to be consumed later. Tried to undo all of that, but must have missed this.
(In reply to Doug Thayer [:dthayer] from comment #26)
> (In reply to :Gijs from comment #25)
> > The only thing I wonder is if it's worth allowing cancel, but perhaps we can
> > just leave that for a followup and play it safe for now.
> 
> Yeah - I specifically added the cancel case in because canceling
> successfully closes the wizard, but the rest of Firefox is unresponsive
> until the promise resolves. We could have cancel force a stop on the
> spinEventLoopUntil call, though.

That seems like a good idea. Review will have to wait until tomorrow (it's 11.30pm here now) but either here or in a follow-up is fine.
Comment on attachment 8942331 [details]
Bug 862127 - Convert migration.js to use async migration interfaces

https://reviewboard.mozilla.org/r/212594/#review219724

r=me with the nits below fixed and a follow-up for making cancel immediately kill the spin loop.

Thanks for working on this!

::: browser/components/migration/content/migration.js:250
(Diff revisions 1 - 2)
>      var dataSources = document.getElementById("dataSources");
>      while (dataSources.hasChildNodes())
>        dataSources.firstChild.remove();
>  
> -    var items = MigrationUtils.spinResolve(this._migrator.getMigrateData(this._selectedProfile,
> +    var items = this.spinResolve(this._migrator.getMigrateData(this._selectedProfile,
>                                                                           this._autoMigrate));

Nit: please fix the alignment of the arguments.

::: browser/components/migration/content/migration.js:361
(Diff revisions 1 - 2)
>      if (this._autoMigrate)
>        this._itemsFlags =
> -        MigrationUtils.spinResolve(this._migrator.getMigrateData(this._selectedProfile,
> +        this.spinResolve(this._migrator.getMigrateData(this._selectedProfile,
>                                                                   this._autoMigrate));
>  

Can you align the arguments for `getMigrateData` "properly" and put braces around the contents of this if block?
Attachment #8942331 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1432529
Blocks: 1432533
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 1bb73e8f20f442bea0b101834942f24469356acb -d f5fe81e72056: rebasing 443753:1bb73e8f20f4 "Bug 862127 - Make migration interfaces more async r=Gijs"
merging browser/components/migration/360seProfileMigrator.js
merging browser/components/migration/ChromeProfileMigrator.js
merging browser/components/migration/EdgeProfileMigrator.js
merging browser/components/migration/FirefoxProfileMigrator.js
merging browser/components/migration/IEProfileMigrator.js
merging browser/components/migration/MigrationUtils.jsm
merging browser/components/migration/SafariProfileMigrator.js
warning: conflicts while merging browser/components/migration/360seProfileMigrator.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/migration/ChromeProfileMigrator.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/migration/FirefoxProfileMigrator.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/migration/IEProfileMigrator.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/migration/SafariProfileMigrator.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c73e6be9d2d
Make migration interfaces more async r=Gijs
https://hg.mozilla.org/integration/autoland/rev/f7a3e9638b6f
Convert migration.js to use async migration interfaces r=Gijs
https://hg.mozilla.org/integration/autoland/rev/e650035e766f
Remove all sync IO from Chrome migration r=Gijs
https://hg.mozilla.org/integration/autoland/rev/ea23e995ded7
Fix migrator tests to use async interfaces r=Gijs
Depends on: 1433024
Blocks: 1438972
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: