1440334 - SyncedBookmarksMirror should use a jankYielder

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

Split off from bug 1435588.

There are a few cases where it is necessary, store() is an obvious one (after bug 1435588 at least), but we probably want to do it during application, and maybe other times as well.

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1440334 - (part 1) Add an iterator wrapper to services-common/async.js that uses a jankYielder for you

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

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1440334 - (part 2) Yield to the event loop more in SyncedBookmarksMirror.

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

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8953187 [details]
Bug 1440334 - (part 1) Add an iterator wrapper to services-common/async.js that uses a jankYielder for you

https://reviewboard.mozilla.org/r/222470/#review228438

Spiffy!

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8953187 [details]
Bug 1440334 - (part 1) Add an iterator wrapper to services-common/async.js that uses a jankYielder for you

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

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8953188 [details]
Bug 1440334 - (part 2) Yield to the event loop more in SyncedBookmarksMirror.

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

Comment 7

[Lina Butler [:lina]]

Comment on attachment 8953188 [details]
Bug 1440334 - (part 2) Yield to the event loop more in SyncedBookmarksMirror.

https://reviewboard.mozilla.org/r/222472/#review228440

Thanks!

I'm wondering if all these periodic dispatches to the main thread are going to hurt us in other ways, and I wish there were an easier way to chunk work...idle dispatch and workers don't *quite* do everything we're looking for, and I suspect the Right Answer is going to end up being something like, "use Rust or C++ and do it off the main thread". We might be able to cut down on this by splitting the merger out, but I'm saying that from intuition, not from experience.

I don't have any other suggestions, though, and your patch should improve things for large trees...so I think we should take it, and decide later if and how we can improve on "sprinkle some `jankYielder`s into our loops". :-)

::: toolkit/components/places/SyncedBookmarksMirror.jsm:60
(Diff revision 1)
>  
>  ChromeUtils.import("resource://gre/modules/Services.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  XPCOMUtils.defineLazyModuleGetters(this, {
> +  Async: "resource://services-common/async.js",

Do you know if we have a policy for code in `toolkit/` depending on `services/`? I guess this mattered more in the past, where `toolkit/` was shared among Thunderbird, SeaMonkey, etc., which didn't build `services/`.

In practice, I don't think this matters, and I already see code in `toolkit/{modules, components/extensions, components/reader, components/telemetry}` that depends on `services/`.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:3039
(Diff revision 2)
>     * Creates a bookmark node from this merged node.
>     *
>     * @return {BookmarkNode}
>     *         A node containing the decided value and structure state.
>     */
> -  toBookmarkNode() {
> +  async toBookmarkNode() {

Since all its inputs are in memory, I wonder if we should consider moving the merger into a chrome worker. (We should, of course, also fix the obvious issues, like the one you found where we repeatedly loop over the same siblings when deduping). I don't know what the cloning overhead is going to be like, and logging is going to be interesting, but that might remove a source of jank and cut down on yields that we need to do.

Comment 8

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8953188 [details]
Bug 1440334 - (part 2) Yield to the event loop more in SyncedBookmarksMirror.

https://reviewboard.mozilla.org/r/222472/#review228440

> Do you know if we have a policy for code in `toolkit/` depending on `services/`? I guess this mattered more in the past, where `toolkit/` was shared among Thunderbird, SeaMonkey, etc., which didn't build `services/`.
> 
> In practice, I don't think this matters, and I already see code in `toolkit/{modules, components/extensions, components/reader, components/telemetry}` that depends on `services/`.

As you mention there's already plenty of code that depends on it. In practice, I can't see why someone would import SyncedBookmarksMirror and expect it to work without `services/*` anyway.

> Since all its inputs are in memory, I wonder if we should consider moving the merger into a chrome worker. (We should, of course, also fix the obvious issues, like the one you found where we repeatedly loop over the same siblings when deduping). I don't know what the cloning overhead is going to be like, and logging is going to be interesting, but that might remove a source of jank and cut down on yields that we need to do.

Agreed, this isn't a great fix.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91499c345038
(part 1) Add an iterator wrapper to services-common/async.js that uses a jankYielder for you r=kitcambridge
https://hg.mozilla.org/integration/autoland/rev/81cc9bdd0bfc
(part 2) Yield to the event loop more in SyncedBookmarksMirror. r=kitcambridge

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/91499c345038
https://hg.mozilla.org/mozilla-central/rev/81cc9bdd0bfc