Closed Bug 1453961 Opened 6 years ago Closed 6 years ago

SyncResultObject.add is janky

Categories

(WebExtensions :: Storage, enhancement)

enhancement
Not set
normal

Tracking

(Performance Impact:?, firefox61 fixed)

RESOLVED FIXED
mozilla61
Performance Impact ?
Tracking Status
firefox61 --- fixed

People

(Reporter: florian, Assigned: leplatrem)

Details

(Keywords: perf, Whiteboard: [fxperf:p3])

Attachments

(1 file)

Profile where this blocked the main thread for ~330ms on a fast macbook pro: https://perfht.ml/2EI1mb9

The poorly performing code (O(n^2)) is https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/services/common/kinto-offline-client.js#909-914
Assignee: nobody → mathieu
Component: Firefox: Common → WebExtensions: Storage
Product: Cloud Services → Toolkit
Whiteboard: [fxperf][qf] → [fxperf:p3][qf]
FYI, the problem was fixed upstream:
https://github.com/Kinto/kinto.js/pull/807

We now need to upgrade the vendored kinto.js lib
Is upgrading the vendored kinto.js something that we can do for 61 before the next uplift? When are Kinto upgrades generally done?
Flags: needinfo?(mathieu)
Whenever we need them ;). At :leplatrem's request, I have cut a release of kinto.js which includes the relevant fix. I tried to launch a try build too, but I can't promise I got the right tasks.
Flags: needinfo?(mathieu)
Attachment #8969527 - Flags: review?(MattN+bmo) → review?(florian)
Comment on attachment 8969527 [details]
Bug 1453961: bump kinto.js version to 11.1.2

https://reviewboard.mozilla.org/r/238290/#review244208

::: services/common/kinto-offline-client.js:915
(Diff revision 1)
> +      entries = [entries];
> +    }
>      // Deduplicate entries by id. If the values don't have `id` attribute, just
>      // keep all.
> -    const deduplicated = this[type].concat(entries).reduce((acc, cur) => {
> -      const existing = acc.filter(r => cur.id && r.id ? cur.id != r.id : true);
> +    const recordsWithoutId = new Set(this[type].filter(record => !record.id));
> +    const recordsById = new Map(this[type].filter(record => record.id).map(record => [record.id, record]));

This seems pretty complicated with 2 filter calls over the whole existing array.

Here is a simpler solution:

const recordWithoutId = new Set();
const recordsById = new Map();
this[type].concat(entries).forEach(record => {
  /* same implemention as in the current patch */

And the .concat works even if entries isn't an array yet.
Comment on attachment 8969527 [details]
Bug 1453961: bump kinto.js version to 11.1.2

https://reviewboard.mozilla.org/r/238290/#review244208

> This seems pretty complicated with 2 filter calls over the whole existing array.
> 
> Here is a simpler solution:
> 
> const recordWithoutId = new Set();
> const recordsById = new Map();
> this[type].concat(entries).forEach(record => {
>   /* same implemention as in the current patch */
> 
> And the .concat works even if entries isn't an array yet.

Thanks for the feedback. I'm not crazy in love with the current implementation in general because we internally maintain an array for this data, and as a result have to convert to/from Maps, potentially numerous times, to ensure uniqueness by ID. I think we would do much better to maintain Maps internally, and expose Arrays if we need to, but that was a more involved change and I didn't really have the time at the moment.

I'm not crazy at the use of `concat` in your solution; it feels like it creates needless copying/GC. (Maybe you know something I don't about how `concat` returns a lazily-instantiated Array or something.) But I agree that there's a commonality about how we treat each record. Using this insight, I've taken another swing at this; it's at https://github.com/Kinto/kinto.js/pull/809. Please feel free to review that as well.
(In reply to Ethan Glasser-Camp (:glasserc) from comment #6)

> I've taken another swing at this; it's at
> https://github.com/Kinto/kinto.js/pull/809. Please feel free to review that
> as well.

Looks good to me, thanks!
Comment on attachment 8969527 [details]
Bug 1453961: bump kinto.js version to 11.1.2

https://reviewboard.mozilla.org/r/238290/#review245750

Thanks!
Attachment #8969527 - Flags: review?(florian) → review+
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(mathieu)
Keywords: checkin-needed
Flags: needinfo?(mathieu)
Keywords: checkin-needed
I have marked the issue as resolved.
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d2ed925b490
bump kinto.js version to 11.1.2 r=florian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d2ed925b490
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Yes, the bug status is changed and well as the status for the fixed version and later when it is uplifted you will get notified by email.
Flags: needinfo?(mathieu)
Please disregard the previous comment.
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(mathieu)
Flags: qe-verify-
Product: Toolkit → WebExtensions
Performance Impact: --- → ?
Whiteboard: [fxperf:p3][qf] → [fxperf:p3]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: