1338800 - Consider being stricter about not allowing history/bookmark/login imports to run in parallel

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Dão Gottwald [:dao]]

<Gijs> dao: https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/MigrationUtils.jsm#330-335
<Gijs> dao: based on that block, I think if we start with passwords and then do history/bookmarks, we could potentially do those simultaneously
<Gijs> dao: but I don't know off-hand if that's possible in practice
<dao> Gijs: we probably shouldn't ever do any of these in parallel if IO-heavy tasks are involved
<Gijs> dao: sure, but the tasks aren't IO-heavy for all browsers...
<dao> but if they are we multiply our problems
<dao> the current setup seems to err on the side of allowing things to run parallel, which doesn't seem like a good idea to me

Comment 1

[:Gijs (out for now; he/him)]

Per some of the data in bug 1346796, I'd like us to fix this just in case it does have an effect.

I'm particularly concerned if there are cases where the existing bookmark/history resources might be 'finished' but async notifications end up appearing after we presume they are finished and that slows us down. Marco, can you take a look at the code in question ( https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/MigrationUtils.jsm#366-373 ) and check if we need to be waiting longer for anything specific?

Comment 2

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Does this issue block somehow bug 1341097? I am asking this in order to determine the right resolution for bug 1341097,  also considering that bug 1346796 is fixed.

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Iulia Cristescu, QA [:IuliaC] from comment #2)
> Does this issue block somehow bug 1341097? I am asking this in order to
> determine the right resolution for bug 1341097,  also considering that bug
> 1346796 is fixed.

No, I think bug 1341097 can be considered resolved as-is. It's not the be-all, end-all of performance fixes to importing browser data, but it helped, and we verified that it did. :-)

Comment 4

[Marco Bonardo [:mak]]

(In reply to :Gijs (out until Monday 20th) from comment #1)
> Marco, can you take a look at the code in question (
> https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/
> MigrationUtils.jsm#366-373 ) and check if we need to be waiting longer for
> anything specific?

I am not sure I understand the question.
The reason to run bookmarks and history sequentially while the others in parallel was that bookmarks and history were considered the most expensive of the bunch. And running those at the same time could easily hit the Sqlite.jsm transaction timeout. By running them sequentially we somehow limited the contemporary I/O.
The main reason to run stuff in parallel is to make it finish sooner. It may sound dumb, but the fact is that when we move from sync to async these batch processes can take twice or thrice the time to complete. Slowing down the browser for a long time, plus the risk of the user closing the session in the middle of the operation (maybe even causing an async shutdown timeout crash) doesn't sound too much appealing.

Off-hand, the only things that can happen after the bookmarks and history promises are done is frecency updates and notifications.

Comment 5

[:Gijs (out for now; he/him)]

(In reply to Marco Bonardo [::mak] from comment #4)
> (In reply to :Gijs (out until Monday 20th) from comment #1)
> > Marco, can you take a look at the code in question (
> > https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/
> > MigrationUtils.jsm#366-373 ) and check if we need to be waiting longer for
> > anything specific?
> 
> I am not sure I understand the question.
> The reason to run bookmarks and history sequentially while the others in
> parallel was that bookmarks and history were considered the most expensive
> of the bunch. And running those at the same time could easily hit the
> Sqlite.jsm transaction timeout. By running them sequentially we somehow
> limited the contemporary I/O.

Right.

> The main reason to run stuff in parallel is to make it finish sooner.

OK, but the problem right now is that the code doesn't guarantee that bookmarks/history imports run without anything else running at the same time, and so we still face IO contention like what you suggest earlier is the reason for running bookmarks/history sequentially.

> Off-hand, the only things that can happen after the bookmarks and history
> promises are done is frecency updates and notifications.

OK. I guess right now the bookmarks promises get resolved without waiting for the frecency updates. I guess this means it might still be worth fixing bug 1346979. It can probably use onManyFrecenciesChanged based on an opt-in that we use from insertTree.

Additionally, maybe in the migrator loop we should (a) ensure earlier tasks have finished before doing history/bookmarks import, and (b) ensure frecency updates have been processed before we continue from bookmarks to history or vice versa.

We've talked before of removing cookie importing altogether. We won't use it for automigration. I think we should take this opportunity to remove it (or at least untick it by default in the wizard, or something).

Beyond that the only other resource we import from external browsers is passwords, at which point what we're doing right now is basically going to be the same as just running all resources sequentially, so then we should just simplify the code to do that for all resources. In any case, it feels like based on the number of passwords we generally import ( https://mzl.la/2mtcunQ ) and how long the import takes (95th percentile is <=0.5s on IE/Edge, <=2s on Chrome ( https://mzl.la/2mt57g4 ), this won't have a significant negative impact on total duration, and might well reduce contention/jankiness.

Does this sound sensible to you, Marco?

Comment 6

[Marco Bonardo [:mak]]

(In reply to :Gijs from comment #5)
> Additionally, maybe in the migrator loop we should (a) ensure earlier tasks
> have finished before doing history/bookmarks import, and (b) ensure frecency
> updates have been processed before we continue from bookmarks to history or
> vice versa.

You could probably do that through onManyFrecenciesChanged. But you may also just check how bad the situation is after frecency changes have been batched, before making migration take much longer. I'm still not totally sure a barely responsive browser for a long time is better than an hung browser for a short time.

> We've talked before of removing cookie importing altogether. We won't use it
> for automigration. I think we should take this opportunity to remove it (or
> at least untick it by default in the wizard, or something).

I think that deserves a separate bug and discussion.
I see how cookies may be useful in the onboarding phase instead, with cookies the browser-to-browser handoff seems like would be smoother. This is something to evaluate, I don't have data to confirm or deny my assumptions. Maybe you do.

> this won't
> have a significant negative impact on total duration, and might well reduce
> contention/jankiness.

I don't see blockers into doing that., Before we had other things to migrate like prefs, fonts and more fancy stuff. Now that the migrated data is a handful of things, we can probably go back to a sequential import, provided we ensure to not block the process on failures on a single entry. Plus now history and bookmarks are likely to happen much faster, so the landscape changed quite a bit. I'd say to go for it.

Comment 7

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1338800 - await completeDeferred for all resources

This shouldn't really have an effect, as bookmarks and history are always
added at the beginning of the resource arrays, and we ensure that each of
those resources finishes before moving on to the next. However, adding this
in still seems like a good idea in case anyone ever puts anything in front
of Bookmarks or History.

Review commit: https://reviewboard.mozilla.org/r/195890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195890/

Comment 8

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1338800 - Wait for frecency updates before moving off bookmarks

Because Bookmarks come before History in the resource lists of the
Chrome migrator and other migrators, we'd like to ensure that
the frecency updates from the Bookmarks migration aren't creating
any contention with the History migration. Ensuring that we await
the frecency updates should be enough to avoid this.

Review commit: https://reviewboard.mozilla.org/r/195892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195892/

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8924662 [details]
Bug 1338800 - Wait for frecency updates before moving off bookmarks

https://reviewboard.mozilla.org/r/195892/#review201334

I'm not convinced waiting for frecency is a win, we should rather change frecency updates to happen on idle timers in the whole product, imo.
I'd suggest for now to throw away the idea, and file a separate bug if we find it to be such a big deal. Both insertTree and history updates now batch frecency updates so the situation may not be so horrible. Note that also history doesn't wait for frecency, so this would only be a partial fix.

::: toolkit/components/places/Bookmarks.jsm:477
(Diff revision 1)
> +              resolve();
> +            },
> +          };
> +          PlacesUtils.history.addObserver(onManyFrecenciesObserver);
> +        });
> +      }

We should not do this, it's adding an observer and each observer slows down any single bookmark notification. The less observers we have around, the better is.
If we really wanted to do this, we should rather change insertBookmarkTree to await on updateFrecency with an if/else.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8924661 [details]
Bug 1338800 - await completeDeferred for all resources

https://reviewboard.mozilla.org/r/195890/#review201336

As discussed in the bug (I understand it's not really clear, not your fault!) let's just make all the resources happen sequentially, the order of history and bookmarks shouldn't matter. The scope of the bug is to avoid running heavy IO tasks in parallel, and since now we have far less migration tasks than in the past, we should go the simple path and then check where we are after this lands.
Basically, change the for loop to await on each resource. completeDeferred should probably be removed, "res.migrate" should become "await res.migrate", the special bookmarks/history condition should be removed.

Comment 11

[Alex Thayer [:alexical] (she/her)]

If we want to get rid of completeDeffered, should we just get rid of the aCallback parameter to the migrate functions entirely? Just awaiting res.migrate won't cut it, since the migrate functions are pretty hit or miss about being async / returning promises. I can update them all to be awaitable in addition to using the callback, it just feels redundant.

Comment 12

[Marco Bonardo [:mak]]

(In reply to Doug Thayer [:dthayer] from comment #11)
> If we want to get rid of completeDeffered, should we just get rid of the
> aCallback parameter to the migrate functions entirely? Just awaiting
> res.migrate won't cut it, since the migrate functions are pretty hit or miss
> about being async / returning promises. I can update them all to be
> awaitable in addition to using the callback, it just feels redundant.

If you remove the callback, the functions will have to return a bool (synchronously or asynchronously) and await can be used in any case. We need the return value to notify itemSuccess.
It's something surely feasible and likely it would bring to simpler code, but it's not necessary for this bug, so maybe it could be a separate cleanup bug (there are various call points that'd need to be fixed and wrapMigrateFunction adds a little bit of complication on the top).

Comment 13

[Alex Thayer [:alexical] (she/her)]

(In reply to Marco Bonardo [::mak] from comment #12)
> If you remove the callback, the functions will have to return a bool
> (synchronously or asynchronously) and await can be used in any case. We need
> the return value to notify itemSuccess.
> It's something surely feasible and likely it would bring to simpler code,
> but it's not necessary for this bug, so maybe it could be a separate cleanup
> bug (there are various call points that'd need to be fixed and
> wrapMigrateFunction adds a little bit of complication on the top).

Okay, in that case I think the best thing to do is just to continue using completeDeferred, since the work to make await res.migrate(resourceDone) be equivalent to await completeDeferred.promise overlaps quite a bit with the work to remove the callback.

Comment 14

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8924661 [details]
Bug 1338800 - await completeDeferred for all resources

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195890/diff/1-2/

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8924661 [details]
Bug 1338800 - await completeDeferred for all resources

https://reviewboard.mozilla.org/r/195890/#review206180

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90b13fe1c2b6
await completeDeferred for all resources r=mak

Comment 17

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/90b13fe1c2b6