1508332 - Move more operations to the RemoteSettings worker

Status: NEW

(Firefox :: Remote Settings Client, enhancement, P3)

Description

[Mathieu Leplatre [:leplatrem]]

Some notes from Bug 1502146, by Gijs:

> moving most of kinto's write operations (including poll/sync stuff) into a worker environment, proving a mirrored-to-main-thread API for consumers. IDB reads already happen OMT and any overhead is IPC/serialization-based, which won't improve by putting it in a worker.

> it would be better if more of this [signature verification] stuff went into the worker, though it sounds like we'd need to create a (chromeonly) webidl constructor for the signature verifier stuff, if one doesn't already exist.

Comment 1

[Mathieu Leplatre [:leplatrem]]

So what happens now is:

• We fetch data (changes since last sync) from the server. This happens async and the data arrives on the main thread
• We fetch local data from indexeddb. This happens async, the data is collected by indexeddb on some other thread and then passed to the main thread
• We pass all this data to the canonical json stringifier in the worker. It passes the result back to the main thread
• We then pass this with the signature information to the signature verifier, which does the work away from the main thread.
• If the signature is correct, we put the changes into indexeddb, and notify observers (on the main thread) about changes.
• If the signature is not correct, we flush the local db, then we fetch all the data from the server (instead of just a diff) and try to verify that.

In an ideal world, this would work something like:

• Tell a worker to sync
• It fetches data from the server
• It fetches the local data from indexeddb
• It does the canonical-json-ifying and the verification
• If successful, it passes the resulting data to the main thread.
• This would avoid something like 3-5 passes of data between threads (?)

Comment 2

[Mathieu Leplatre [:leplatrem]]

Florian,
In order to evaluate the importance of this refactor, we'd like to know how bad it is...

Could you please measure the impact on the reference hardware?

Executing these lines should be enough:

const { RemoteSettings } = ChromeUtils.import(
  "resource://services-settings/remote-settings.js"
);

await RemoteSettings.clearAll();

await RemoteSettings.pollChanges({ full: true });

Thanks!

Comment 3

[Florian Quèze [:florian]]

Here's a profile of the code from comment 2 on the reference hardware: https://perfht.ml/2xdSBYf

The slowest part currently seems to be the processing that happens in Blocklist.jsm: https://perfht.ml/2xg2HrL (note: this is slightly janky even on fast hardware: https://perfht.ml/2PNYOAE)

Comment 4

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] from comment #3)

Here's a profile of the code from comment 2 on the reference hardware: https://perfht.ml/2xdSBYf

The slowest part currently seems to be the processing that happens in Blocklist.jsm: https://perfht.ml/2xg2HrL (note: this is slightly janky even on fast hardware: https://perfht.ml/2PNYOAE)

I'm a bit confused - you're filtering on JS here, and if you look at the complete stack, there's also a lot of time (like, 80ms) being spent deserializing IPC stuff within the indexeddb implementation, right? (ni for this)

As for the JS, it's very hard to work out which bits are actually slow from that profile, because the stacks are broken (_getEntry is in there twice, and in one instance is apparently causing us to load XPIDatabase.jsm (well, time is spent in its global), which doesn't make sense because it's not imported from Blocklist.jsm).

Inverting the stacks, most of the time is spent in filterItem, which is a callback run by remotesettings to remove items whose versions/products/OSes don't apply to the current OS/version/product.

An obvious solution here would be not running those for add-ons anymore. :TheOne, do we still have OS-specific add-on blocks at this point? And is the collection that we're using in kinto Firefox-specific anyway? (that is, we don't have to filter out thunderbird entries or something)

Comment 5

[:Gijs (he/him)]

FWIW, I'm also confused about where the IO is in this profile. It looks like the time filters from comment #3 exclude the clear() call and its effects, but I'm not sure what else they exclude - basically I'm skeptical that fetching 1mb of json data over the network / from disk, and writing that to the indexeddb is 0-cost on the main thread besides the JS we use to process it - I'd expect to see the JSON parsing, at a minimum.

Comment 6

[Andreas Wagner [:TheOne] [use NI]]

(In reply to :Gijs (he/him) from comment #4)

... :TheOne, do we still have OS-specific add-on blocks at this point? And is the collection that we're using in kinto Firefox-specific anyway? (that is, we don't have to filter out thunderbird entries or something)

We generally don't do os-specific add-on blocks anymore and I don't recall a case in 2019 where we did.
The collection in kinto is Firefox-specific by convention. For over a year, we have been using a client-side helper that validates each to-be-blocked add-on GUID against AMO, and if it doesn't exist on AMO, then it's not going to be added to the block.

Comment 7

[:Gijs (he/him)]

(In reply to Andreas Wagner [:TheOne] [use NI] from comment #6)

(In reply to :Gijs (he/him) from comment #4)

... :TheOne, do we still have OS-specific add-on blocks at this point? And is the collection that we're using in kinto Firefox-specific anyway? (that is, we don't have to filter out thunderbird entries or something)

We generally don't do os-specific add-on blocks anymore and I don't recall a case in 2019 where we did.
The collection in kinto is Firefox-specific by convention. For over a year, we have been using a client-side helper that validates each to-be-blocked add-on GUID against AMO, and if it doesn't exist on AMO, then it's not going to be added to the block.

Great. I forgot one more thing - what about application (not add-on) versions? It used to be that some blocks were valid for Firefox 3 but not Firefox 4, for instance. Do we still support those? If not, I think we can get rid of almost all the filtering code. I talked to Andreas on slack and he suggested to ask :eviljeff...

Comment 8

[Andrew Williamson [:eviljeff]]

what about application (not add-on) versions? It used to be that some blocks were valid for Firefox 3 but not Firefox 4, for instance. Do we still support those? If not, I think we can get rid of almost all the filtering code.

In the v3 blocklist we don't support applications or application versions. However, there may be some old blocks in the existing blocklist that still specify specific application versions in the existing blocklist though (I see there are some application specific blocks in https://firefox.settings.services.mozilla.com/v1/buckets/blocklists/collections/addons/records at least).

@jorgev to confirm the plans for cleaning up the existing blocklist of non-Firefox blocks and/or blocks that target specific versions of Firefox.

Comment 9

[Florian Quèze [:florian]]

(In reply to :Gijs (he/him) from comment #4)

(In reply to Florian Quèze [:florian] from comment #3)

Here's a profile of the code from comment 2 on the reference hardware: https://perfht.ml/2xdSBYf

The slowest part currently seems to be the processing that happens in Blocklist.jsm: https://perfht.ml/2xg2HrL (note: this is slightly janky even on fast hardware: https://perfht.ml/2PNYOAE)

I'm a bit confused - you're filtering on JS here, and if you look at the complete stack, there's also a lot of time (like, 80ms) being spent deserializing IPC stuff within the indexeddb implementation, right? (ni for this)

I looked at the first profile (https://perfht.ml/2xdSBYf) then looked for the jank markers, saw that the biggest one was on a yellow (ie. JS) area of the profile, and that's the reason why I filtered for JS-only. Other stuff likely has costs too. I was just looking for the worst offender in terms of responsiveness.

There's actually some visible indexedDB activity in the profile: https://perfht.ml/2PPQgcB

As for the JS, it's very hard to work out which bits are actually slow from that profile, because the stacks are broken (_getEntry is in there twice, and in one instance is apparently causing us to load XPIDatabase.jsm (well, time is spent in its global), which doesn't make sense because it's not imported from Blocklist.jsm).

Lots of the JS stacks are confusing because of the use of the await keyword. When we resume, the JS stack starts from InterpretGeneratorResume. We are seeing time spent in XPIDatabase.jsm because of the properties it adds through defineAddonWrapperProperty on the add-ons.

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

Yes, blocklist v3 won't include entries that don't target current versions of Firefox. Our plan is to eventually freeze the previous blocklist, so that older clients can still use it. Once we freeze it, a static file should suffice. A more detailed migration plan can be found here.

If you're looking into dropping these features earlier, let me know and we can look into the current blocks that use them. We may be able to drop them or change them to work around it.

Comment 11

[:Gijs (he/him)]

I filed bug 1620580 for removing the filtering for add-ons.

I don't know how to assess if there's something to be gained, performance-wise, by moving more operations to the worker here - I can't find the relevant stuff in the profile; perhaps profiles that included the worker would shed more light on the order in which certain things are happening and where we're paying costs.

I also don't know how any changes here would affect the new version of the blocklist - are we planning to use remote settings attachments? If so, does the data still go through some kind of signature validation that passes through the worker?

Comment 12

[Rob Wu [:robwu]]

(In reply to :Gijs (he/him) from comment #11)

I also don't know how any changes here would affect the new version of the blocklist - are we planning to use remote settings attachments?

Yes, we're going to use remote settings attachments. I'm working on it right now, there will be a set of patches next week (I'll add it to bug 1620621).

If so, does the data still go through some kind of signature validation that passes through the worker?

Since I'm using RemoteSettings to retrieve the attachment, the answer is probably yes: https://searchfox.org/mozilla-central/rev/13b081a62d3f3e3e3120f95564529257b0bf451c/services/settings/Attachments.jsm#80
It is an internal implementation detail of RemoteSettings, so you should be able to refactor the code without breaking my use case.

Comment 13

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #12)

(In reply to :Gijs (he/him) from comment #11)

If so, does the data still go through some kind of signature validation that passes through the worker?

Since I'm using RemoteSettings to retrieve the attachment, the answer is probably yes: https://searchfox.org/mozilla-central/rev/13b081a62d3f3e3e3120f95564529257b0bf451c/services/settings/Attachments.jsm#80
It is an internal implementation detail of RemoteSettings, so you should be able to refactor the code without breaking my use case.

Ah, thanks for the pointer. This looks less problematic though because we just pass the URL to the worker and get a bool back, so there isn't 1mb of data being passed back and forth...

Comment 14

[Florian Quèze [:florian]]

Here is a subset of a startup profile captured on try (Linux ccov build) that shows messages sent between the parent process and its DOM Workers: https://bit.ly/2WMf5KC