1323867 - Bump bookmark validation threshold to 1k

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Lina Butler (ex-Mozilla)]

It's currently 100 bookmarks. We may want to yield to the event loop in the validation code, to minimize jank. Ride this to Aurora.

Comment 1

[Lina Butler (ex-Mozilla)]

We may also want to have validation ride all the way to Release.

Comment 2

[Lina Butler (ex-Mozilla)]

Bump first, measure, then make a decision to ride the trains later.

Comment 3

[Lina Butler (ex-Mozilla)]

Bumping the pref is a one-line change, but I noticed a bit of jank with my profile of ~2.4k bookmarks. I suspect we'll want to periodically yield to the event loop, just like we discussed at triage.

Comment 4

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1323867 - Bump bookmark validation threshold to 1k.

This patch also refactors `compareServerWithClient` and
`inspectServerRecords` to periodically yield back to the main thread,
since validating a large number of bookmarks in a tight loop causes
jank. Yielding every 50 records for 50ms is somewhat arbitrary, but
keeps things snappy for my profile (2947 records).

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

Comment 5

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

Comment on attachment 8826705 [details]
Bug 1323867 - Bump bookmark validation threshold to 1k.

https://reviewboard.mozilla.org/r/104606/#review105984

Looks good, but the conversion of some of those functions to generators will break TPS, and [aboutsync](https://github.com/mhammond/aboutsync)... Not breaking TPS is required IMO for landing this, but keeping aboutsync working is a "nice to have".

If you do decide to handle aboutsync, it should be done in a way that won't break on older firefoxes. I believe you can check if a function is a generator via it's constructor property... e.g. `generator.constructor.name === "GeneratorFunction"`, although maybe there's a better way. This kind of sucks, admittedly.

Comment 6

[Lina Butler (ex-Mozilla)]

Comment on attachment 8826705 [details]
Bug 1323867 - Bump bookmark validation threshold to 1k.

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

Comment 7

[Lina Butler (ex-Mozilla)]

Now that we can use async functions in production code, I changed the validator to use them instead of tasks. Still seeing a race in test_bookmark_conflict.js; I'll file a separate bug for that. The Sync ping schema change is also to fix TPS. I submitted a PR for About Sync here: https://github.com/mhammond/aboutsync/pull/23

Comment 8

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

Comment on attachment 8826705 [details]
Bug 1323867 - Bump bookmark validation threshold to 1k.

https://reviewboard.mozilla.org/r/104606/#review110430

lgtm

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8826705 [details]
Bug 1323867 - Bump bookmark validation threshold to 1k.

https://reviewboard.mozilla.org/r/104606/#review110926

::: services/sync/modules/bookmark_validator.js:8
(Diff revision 2)
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  const Cu = Components.utils;
> +const Timer = Components.Constructor("@mozilla.org/timer;1", "nsITimer");

As a meta-comment (as I don't really care!), note there is a Timer.jsm new code tends to use

Comment 10

[Lina Butler (ex-Mozilla)]

Comment on attachment 8826705 [details]
Bug 1323867 - Bump bookmark validation threshold to 1k.

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

Comment 11

[Lina Butler (ex-Mozilla)]

Comment on attachment 8826705 [details]
Bug 1323867 - Bump bookmark validation threshold to 1k.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104606/diff/3-4/

Comment 12

[Lina Butler (ex-Mozilla)]

Comment on attachment 8826705 [details]
Bug 1323867 - Bump bookmark validation threshold to 1k.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104606/diff/4-5/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c3b1968e183
Bump bookmark validation threshold to 1k. r=tcsc

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/074d6b728c5c
followup, placate eslint

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c3b1968e183
https://hg.mozilla.org/mozilla-central/rev/074d6b728c5c