Closed Bug 1502146 Opened 6 years ago Closed 6 years ago

kinto shouldn't do heavy weight operations in the main thread of the parent process

Categories

(Cloud Services :: Firefox: Common, enhancement)

Product:
Cloud Services
Component:
Firefox: Common
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Performance Impact:medium, firefox65 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 65
Project Flags:
Performance Impact medium
Tracking Flags:
Tracking Status
firefox65 --- fixed

People

(Reporter: smaug, Assigned: leplatrem)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: perf:responsiveness, Whiteboard: [fxperf:p2])

Attachments

(1 file)

Bug 1502146 - Reduce impact of RemoteSettings synchronization on main thread r=Gijs
47 bytes, text/x-phabricator-request
Details | Review 
Could it use a worker to do whatever it does?
Summary: kinto shouldn't do heavy weight operation in the main thread of the parent process → kinto shouldn't do heavy weight operations in the main thread of the parent process
> Could it use a worker to do whatever it does?

I guess so, but I'd need some help or some example, I'm not familiar with those at all...
I believe we could move the polling to a «worker», since AFAIK there's absolutely no need to be in the main thread (we just populate a chrome IndexedDB).

Currently, all we do is triggered by a timer:
https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/services/settings/RemoteSettingsComponents.js

Do you have examples of timers that run in a worker?

Thanks!
Flags: needinfo?(bugs)
Just use setTimeout.
Flags: needinfo?(bugs)
Use setTimeout() to prevent RemoteSettings polling to run from the main thread of the parent process
Assignee: nobody → mathieu
Version: unspecified → Trunk
I meant that you can use setTimeout in workers.
> I meant that you can use setTimeout in workers.

Do you have something particular in mind? The code is already full of promises, and the units of work are already "atomic" AFAIK (IndexedDB writes for example).

> I don't understand this. Maybe @smaug can comment. But just using a setTimeout here doesn't change anything - this code is already on a timeout. The bug got filed because the sync operation runs on the main thread, and setTimeout alone doesn't change that at all.

At one point I thought that it sounded too easy ;) 

Gijs, do you have examples in mind where a timer notification is offloaded to a worker? Would it be enough to use the idle queue with `Services.tm.idleDispatchToMainThread()`?

Thanks for your precious help :)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Mathieu Leplatre [:leplatrem] from comment #7)
> > I meant that you can use setTimeout in workers.
> 
> Do you have something particular in mind? The code is already full of
> promises, and the units of work are already "atomic" AFAIK (IndexedDB writes
> for example).
> 
> > I don't understand this. Maybe @smaug can comment. But just using a setTimeout here doesn't change anything - this code is already on a timeout. The bug got filed because the sync operation runs on the main thread, and setTimeout alone doesn't change that at all.
> 
> At one point I thought that it sounded too easy ;) 
> 
> Gijs, do you have examples in mind where a timer notification is offloaded
> to a worker? Would it be enough to use the idle queue with
> `Services.tm.idleDispatchToMainThread()`?

That still wouldn't really get things off the main thread, it'd just try to run them when it's less busy - but if it's liable to hold up the main thread for longer periods of time (an idle slice is just a few ms) then it's still likely to cause user-perceptable jank.

I don't know what the state of the art way of using workers is, these days. The only example that I am vaguely familiar with is how reader mode uses them:

https://searchfox.org/mozilla-central/source/toolkit/components/reader/ReaderWorker.js#5

this file gets loaded from https://searchfox.org/mozilla-central/source/toolkit/components/reader/ReaderWorker.jsm , and called from https://searchfox.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm#385 .

You could presumably do something similar for kinto and have the main thread `post` to a worker that does the actual indexed db and filtering work, and posts a message when it's completed, which should probably be used to ensure we re-run that process if the browser quits before the process is complete, or something.
Flags: needinfo?(gijskruitbosch+bugs)
> I don't know what the state of the art way of using workers is, these days. The only example that I am vaguely familiar with is how reader mode uses them

Excellent, thanks! 

> You could presumably do something similar for kinto and have the main thread `post` to a worker that does the actual indexed db and filtering work, and posts a message when it's completed, which should probably be used to ensure we re-run that process if the browser quits before the process is complete, or something.

Honestly, I don't plan on moving the whole API to workers (yet). For now, I'd like to keep things as simple as possible, and let the caller be responsible.

I will look to use a worker for the scheduled timer though. 

Thanks for the guiding ;)
Kinto has showed up badly in some performance profiles, so making it behave better should be quite high priority.

If IDB handling or the heavy JS usage isn't moved to a worker, then nothing needs to be moved there.

When the performance issues occur, are they because of some particular usage patterns? Should all the use of Kinto reviewed from performance point of view and fix problematic cases?
> Kinto has showed up badly in some performance profiles, so making it behave better should be quite high priority.

So far I didn't see much related to Kinto beyond the IDB operations, but I'm open to suggestions.
BTW I am glad that we touched on a sore point, like Bug 1499550

Also, we understand that setting a high priority is an important step in the resolution of an issue. But as I beg in Comment 1 and Comment 2, some examples of existing solutions to similar problems would help better.

> If IDB handling or the heavy JS usage isn't moved to a worker, then nothing needs to be moved there.

I said I was going to move the polling for changes, which leads to a lot of IDB writes. Especially because when a remote collection does not exist locally, we load many entries from a JSON dump into the DB.
It was the case in Bug 1502145 from what I could see in the perfs graph (that I don't fully understand BTW).

> When the performance issues occur, are they because of some particular usage patterns? Should all the use of Kinto reviewed from performance point of view and fix problematic cases?

Moving the IDBObjectStore.put() to a worker will help as far as I could understand.

If some caller iterates a huge IDB collection from the main thread, or if the IDB implementation is not efficient, then the problem should probably be tackled in their respective component IMHO.
Whiteboard: [qf] → [qf][fxperf]
Whiteboard: [qf][fxperf] → [qf:p2:responsiveness][fxperf]
The worker approach looks like a dead end: I cannot import our helpers/abstractions easily (require.js), and I couldn't find any worker example that execute promises (the promise worker test suite does not contain anything like it either).

However, I may have found a way to improve the JSON import. I was wrong when I said «the units of work are already "atomic"».

We do something like this (roughly):

```js
records.forEach(r => store.put(r))
```

And from what I could read on the Web, it would help greatly if we'd do:

```js
let i == 0;

putNext();

function putNext() {
  if (i == records.length) {
    return;
  }
  store.put(records[i]).onsuccess = putNext;
}
```

We'll cut a new release of the kinto-offline-client and see how it improves the situation.
I recorded some profiles for the following actions on a blank profile.

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

On trunk:
http://bit.ly/2yKpJVs

And with with the change described in Comment 12:
http://bit.ly/2OhWqhY


It looks like there is not as much correlation between the main thread delays and the calls to loadDump().

Could you please confirm?
Flags: needinfo?(bugs)
FWIW, Promises work just fine in workers.

The first profile has very long hang while kinto is doing something with IndexedDB.
In the other profile it seems to be split to several pieces, so better, but still using lots of time.

In both profiles importing kinto-offline-client.js takes also lots of time

So in both cases lots of time is spent in the main thread of the parent process - which is the worst possible place to do any heavy operations.

What all would need to be run in workers to get kinto working there?
Flags: needinfo?(bugs)
> FWIW, Promises work just fine in workers.

I could not find any existing code where a worker runs a promise. 

And I don't have the Gecko skills to figure out how to tackle this on my own. 
I tried naively and was having errors about promises not being serializable etc. 

> In both profiles importing kinto-offline-client.js takes also lots of time

It has quite a few `ChromeUtils.import`, I'll replace them with `ChromeUtils.defineModuleGetter`, it should improve right?

> So in both cases lots of time is spent in the main thread of the parent process - which is the worst possible place to do any heavy operations.

As said in previous comments, our code is ran by a timer every 24h. We absolutely don't need to be in the main thread, it should be a background task. But it's the default and we didn't do any extra effort. Maybe we could have a parameter for the timer API to truely run in the background?

> What all would need to be run in workers to get kinto working there?

If someone can show me a dummy example where a worker opens a chrome IndexedDB, I will be able to rewrite our code so that all the heavy parts run in a worker. But without an example I feel like a fish out of water :(

Thanks for your feedback :)
If a user installs FF, does the code run soon after startup (and then next time in 24h)?
> If a user installs FF, does the code run soon after startup (and then next time in 24h)?

The heavy code (basically importing JSON dumps into IndexedDB) can be initiated from several places:

- the update timer (https://searchfox.org/mozilla-central/rev/39cb1e96cf97713c444c5a0404d4f84627aee85d/services/settings/servicesSettings.manifest#7) which I guess is smart enough to figure out that it should run not too long after startup on new profiles
- a push server notification (https://searchfox.org/mozilla-central/rev/39cb1e96cf97713c444c5a0404d4f84627aee85d/services/settings/remote-settings.js#849-874) which is triggered soon after startup as far as I understood
- a call to RemoteSettings().get() by some component, if the locaan empty database for which there is a local JSON dump

Once it ran, the next runs are not doing anything.
> if the locaan empty database for which there is a local JSON dump

if the local database is empty, and there is a local JSON dump
I updated my patch:
- now load dependencies lazily in kinto-offline-client.js
- chain put operations as described in Comment 12
- added a worker to run the canonical stringification (since it runs synchronously)

As asked in Comment 15, with an example of async tasks in a worker, I could move more stuff there.

The new profile with this patch is:
https://perfht.ml/2Or1SiG
(In reply to Mathieu Leplatre [:leplatrem] from comment #20)
> I updated my patch:
> - now load dependencies lazily in kinto-offline-client.js
> - chain put operations as described in Comment 12
> - added a worker to run the canonical stringification (since it runs
> synchronously)
> 
> As asked in Comment 15, with an example of async tasks in a worker, I could
> move more stuff there.
> 
> The new profile with this patch is:
> https://perfht.ml/2Or1SiG

I still see all the put() operations taking 800ms.

(In reply to Mathieu Leplatre [:leplatrem] from comment #15)
> If someone can show me a dummy example where a worker opens a chrome
> IndexedDB, I will be able to rewrite our code so that all the heavy parts
> run in a worker. But without an example I feel like a fish out of water :(

I don't really understand - IndexedDB is available on a worker global the same way it is on the non-worker global. Same with fetch(). You could move all the operations into the worker, including fetch()-ing updates and then putting them in the DB. What, specifically, are you getting stuck on?
Flags: needinfo?(mathieu)
> I still see all the put() operations taking 800ms.

This is sad I know but I don't think we do anything wrong :/

> You could move all the operations into the worker, including fetch()-ing updates and then putting them in the DB. What, specifically, are you getting stuck on?

Well, mainly this:

```
0:01.58 pid:13051 [13051, DOM Worker] WARNING: 'aRv.Failed()', file firefox/dom/workers/WorkerPrivate.cpp, line 4412
0:01.58 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "DataCloneError: The object could not be cloned." {file: "resource://services-settings/RemoteSettingsWorker.js" line: 34}]
postMessage@resource://gre/modules/PromiseWorker.jsm:313:17
Async*post@resource://gre/modules/PromiseWorker.jsm:253:28
loadDumpFile@resource://services-settings/remote-settings.js:234:22
```

If the agent method is async, the promise is not waited. 

I tried to grope for a solution where the worker would send a message, but since I can't use anything with ChromeUtils/Services I'm stuck..
Flags: needinfo?(mathieu)
From a quick glance, this seems like some low-hanging fruit to take some work off of the main thread. We should work with leplatrem and the team to get this stuff off of the main thread, so setting to fxperf:p2 to address this sooner rather than later.
Whiteboard: [qf:p2:responsiveness][fxperf] → [qf:p2:responsiveness][fxperf:p2]
> We should work with leplatrem and the team to get this stuff off of the main thread, so setting to fxperf:p2 to address this sooner rather than later.

Thanks! Who should I ping to get some help?

Should we at least ship what was done in the current attached patch? It's already an improvement...

> I still see all the put() operations taking 800ms.

:asuth, does it look normal to you that the internal ``mozilla::dom::IDBObjectStore::AddOrPut()`` takes so long on my fancy laptop? Should we open a dedicated issue?
https://perfht.ml/2Or1SiG

Thanks all!
Flags: needinfo?(bugmail)
(In reply to Mathieu Leplatre [:leplatrem] from comment #24)
> :asuth, does it look normal to you that the internal
> ``mozilla::dom::IDBObjectStore::AddOrPut()`` takes so long on my fancy
> laptop? Should we open a dedicated issue?
> https://perfht.ml/2Or1SiG

Thanks for the profile!

It looks like the costs are basically:
- 2/3: Our ValueWrapper structured cloning of the payload for index traversal purposes for compliance with the spec in the face of getters.
- 1/3: The cost of structured clone serialization into the disk representation for IndexedDB.

The ValueWrapper cloning is a somewhat new cost as we were violating the spec for a while.  I have to admit to being surprised at its relative cost, although I shouldn't be.  We're duplicating an entire object graph because that's what the spec says and our structured clone implementation gives us.  We will absolutely take a look at whether we can optimize this using our prior knowledge of the traversal paths prior to performing the (conceptual) structured clone, but it's very non-trivial and would happen in 2019Q1 at the earliest.  (In particular, structured cloning explicitly triggers getters as part of the process, and these getters run in the global with the ability to side-effect which gives them the ability to impact parts of the object sub-graph that are part of the traversed index/key-path sub-graph.  So to make this more efficient we need to use a custom serialization step that still performs the entire object traversal, but that allows aus to reduce the subsequent deserialization or key-extraction steps.)


Your best bet is absolutely to move everything you can to workers.  The caveat is if you end up having to postMessage a bunch of things to/from the main-thread, then you might not see the savings you want because postMessage()'s structured clone is effectively the same cost as the IndexedDB add/put structured clone.  It sounds like that's feasible here since a JSON file is being consumed.  Although if you do need to access things that are currently XPConnect only, such as things exposed via Services, then it may be necessary for those Services to either:
- Be exposed via WebIDL if they're implemented in C++/Rust so they can be directly exposed to the Worker.  (WebIDL can be exposed to workers, XPCOM-via-XPConnect cannot.)
- Be moved to ChromeWorkers themselves if they're currently main-thread JS, and communicate with your worker via MessageChannel MessagePorts (which bypasses the main-thread).  This may in turn require those things migrated to ChromeWorkers have their C++/Rust dependencies moved to exposing things via WebIDL.

I'm happy to discuss some of the problem space via Vidyo if that expedites things.  Scheduling likely works best via my asuth@moco email.
Flags: needinfo?(bugmail)
(In reply to Mathieu Leplatre [:leplatrem] from comment #22)
> Well, mainly this:
> 
> ```
> 0:01.58 pid:13051 [13051, DOM Worker] WARNING: 'aRv.Failed()', file
> firefox/dom/workers/WorkerPrivate.cpp, line 4412
> 0:01.58 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "DataCloneError:
> The object could not be cloned." {file:
> "resource://services-settings/RemoteSettingsWorker.js" line: 34}]
> postMessage@resource://gre/modules/PromiseWorker.jsm:313:17
> Async*post@resource://gre/modules/PromiseWorker.jsm:253:28
> loadDumpFile@resource://services-settings/remote-settings.js:234:22
> ```

Please make sure that anything you're trying to postMessage from the worker or back is allowed by the structured clone algorithm.  https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm has a great reference on this.

In particular Error instances won't serialize and Promises absolutely won't serialize.
(In reply to Andrew Sutherland [:asuth] from comment #25)
> Your best bet is absolutely to move everything you can to workers.  The
> caveat is if you end up having to postMessage a bunch of things to/from the
> main-thread, then you might not see the savings you want because
> postMessage()'s structured clone is effectively the same cost as the
> IndexedDB add/put structured clone.  It sounds like that's feasible here
> since a JSON file is being consumed.

Right, I would expect it to be possible to move the reading of the JSON as well as the sync-ing fetches into the worker. Then you no longer have the cost of the structured clone because everything is in the worker, and you don't need to postMessage - though of course you'll still need to postMessage to get stuff out of the worker.
Just popping in here per mconley's suggestion. Do chat with asuth if you haven't already, but if you need any further help with the details on this or could use an extra set of hands feel free to reach out to me!
Thanks all!

I will read carefully the previous comments, but my first question would be: how to wait for the worker to finish an async task? Shall they exchange messages (with some sort of state machine)?

For example, the worker will fetch resource and perform IDB operations, but from the caller standpoint ideally it could just be:

```js
await new Promise((resolve) => worker.post("importDump", [url, resolve]));
```

And in the worker:

```js
function dispatch(method, args = []) {
  return {
    importDump(url, done) {
      fetch(url)
        .then(r => r.json())
        .then(data => {
          indexedDB.open();
          ... 
        .then(() => {
          ...
        })
        .then(done);
    }  
  }[method](...args)
}
```
I think what you really want is just an association of IDs to closures. You of course can't call that `resolve` callback directly from the worker, because you can't share state between threads in JS, but you could smooth over that with an abstraction (unfortunately I don't know if this abstraction already exists - last time I had to do this I don't think it did.)

The abstraction would look something like this:

```js
await new Promise((resolve) => callOnWorker("importDump", [url], resolve));
```

```js
let lastCallbackId = 0;
let callbacks = new Map();
let innerWorker = new ChromeWorker(...);

function callOnWorker(name, args, callback) {
  callbacks.set(++lastCallbackId, callback);
  innerWorker.postMessage([name, lastCallbackId, args]);
}

innerWorker.onmessage = function(e) {
  let [callbackId, args] = e.data;
  callbacks.get(callbackId)(...args);
};
```

Inside the worker would be a similar dance. Hopefully that makes sense / is what you're asking for? (Excuse any typos or stylistic problems.)
(The above is a bit naive - you'll need to make sure the worker closes eventually and that your callbacks are deleted, etc.)
(In reply to Doug Thayer [:dthayer] from comment #30)
> (unfortunately I don't know if this
> abstraction already exists - last time I had to do this I don't think it
> did.)

That reminded me of this:

https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/toolkit/components/promiseworker/PromiseWorker.jsm

Maybe this is what we're looking for? I've never used it before, but from a glance it looks like what we want? (Although it wraps a ChromeWorker instead of a standard WebWorker).
It looks like the patch on try is already using a BasePromiseWorker, and yeah that looks like what we want. So I'm not sure what exactly Mathieu is running into. Mathieu, are you trying to pass promises or a callback directly in the second argument to BasePromiseWorker.post?
> Mathieu, are you trying to pass promises or a callback directly in the second argument to BasePromiseWorker.post?

Well, passing a callback was just an idea to work around the limitations of available abstractions. Obviously it does not work for serialization reasons.
And as far as I understood, passing a promise will just resolve it before calling the worker which does not appear to be useful here :)

What I want is to transparently detach some resource consuming task to a worker.

```js
// remote-settings.js

let timestamp = await db.getLastModified();

if (!timestamp) {
  // Execute the worker and wait.
  await worker.post("importDump", [url]));
  // Worker has finished here.
  timestamp = await db.getLastModified();
}

// go on...

```

And on the worker side, be able to execute series of promises as shown in Comment 29


I will see if I can make it work with your proposal!

It look like:

```js
await new Promise((resolve) => {
  callbacks.set(++lastCallbackId, resolve);
  worker.post("importDump", [url, lastCallbackId]);
});

```

Some hand holding feels good :) Thanks!
Alright, I managed to do something by getting rid of the PromiseWorker abstraction and using raw ChromeWorker.

The profile looked like this with CanonicalJSON running in the main thread:
https://perfht.ml/2yVJKbs

And if I grabbed it correctly, the current version of the patch looks like this:
https://perfht.ml/2OpQ6Fb
Mathieu, am I understanding correctly that your problem with using PromiseWorker is it doesn't allow you to use promises within the worker code itself? I think you're right in that. PromiseWorker expects the worker code to synchronously process things - it only allows for promises in the calling code. In that case it might be best to simply extend PromiseWorker in worker/PromiseWorker.js to handle promises. Maybe we could extend the Meta type that it uses or something.

In any case, it looks like there's still a good chunk of things that we want to move into the worker here, no? For example, I think we'll need to move the loadJSONDump call into the worker as well, otherwise we spend a huge chunk of time serializing that data.
> Mathieu, am I understanding correctly that your problem with using PromiseWorker is it doesn't allow you to use promises within the worker code itself?

Yes indeed.

> In that case it might be best to simply extend PromiseWorker in worker/PromiseWorker.js to handle promises. Maybe we could extend the Meta type that it uses or something.

I'm not entirely confident to be able to do that honestly. I would be happy to rewrite my patch to use that new thing though :) 

> it looks like there's still a good chunk of things that we want to move into the worker here, no? For example, I think we'll need to move the loadJSONDump call into the worker as well, otherwise we spend a huge chunk of time serializing that data.

In the current version of the patch loadJSONDump is done in the worker.

We could move theoretically move everything to the worker, but most our IndexedDB are implemented in a library that we ship as a bundle (kinto-offline-client.js). Since it contains ChromeUtils and XPCOMUtils stuff, we can't import it from the worker.

I duplicated some code for the importDump part. That's acceptable but I wouldn't want to duplicate more code. We could also bundle the lib in pure JS, but that would mean to ship it twice in the code base, since it's used elsewhere in mozilla-central.

I am open to suggestions if someone has a strong opinion :)
You could use importScripts() and ChromeUtils.import() to load the same script, so you could factor out common code into a script that the module and the worker load using those different importing tools?
(In reply to Mathieu Leplatre [:leplatrem] from comment #35)
> Alright, I managed to do something by getting rid of the PromiseWorker
> abstraction and using raw ChromeWorker.
> 
> The profile looked like this with CanonicalJSON running in the main thread:
> https://perfht.ml/2yVJKbs
> 
> And if I grabbed it correctly, the current version of the patch looks like
> this:
> https://perfht.ml/2OpQ6Fb

I don't see a significant difference in cost between these profiles. In fact, if I just search for 'kinto' when looking at the main thread, I see more cost in the after- profile than in the before- profile. The difference seems to mostly be noise. But I see the IDB stuff still taking up time in the profile. I don't really understand this based on the discussion so far. Andrew, can you take a look?
Flags: needinfo?(bugmail)
I'm having trouble finding the interesting bits.  I'd suggest re-running the profiler and making sure to do the following:
- Set the "dom.indexedDB.logging.profiler-marks" pref to true so IndexedDB puts markers in the profiler log.
- Add "Worker" to the threads filter for the profiler extension so you get your worker in the profile.
- Instrument your code with calls to Performance.mark()[1] and/or Performance.measure()[2] since they end up in the profile as well[3] (as long as the build had MOZ_GECKO_PROFILER defined, but that's a pre-condition everywhere).

1: https://developer.mozilla.org/en-US/docs/Web/API/Performance/mark
2: https://developer.mozilla.org/en-US/docs/Web/API/Performance/measure
3: https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/dom/performance/Performance.cpp#257
https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/dom/performance/Performance.cpp#360
Flags: needinfo?(bugmail)
Thanks Andrew and Gijs for your help!

Here is a recap:

Before, changeset f97d54f24e02, Mon Nov 12 11:58:52 2018 +0200:
https://perfht.ml/2z9cJZQ
kinto takes +1200ms of the main thread.

After, with the current version of the patch:
https://perfht.ml/2z8Mdzw
kinto takes ~380ms of the main thread.

I would say that I'm done with the low hanging fruits. I would suggest to open another ticket in order to offload more stuff. That would probably imply a lot of refactoring (as suggested in Comment 39)
I realize that rewriting the ``omitKeys()`` function, from this:

```js
function omitKeys(obj, keys = []) {
  return Object.keys(obj).reduce((acc, key) => {
    if (!keys.includes(key)) {
      acc[key] = obj[key];
    }

    return acc;
  }, {});
}
```

to this:

```js
function omitKeys(obj, keys = []) {
  const result = {};
  for (const k in obj) {
    if (keys.includes(k)) {
      continue;
    }
    result[k] = obj[k];
  }
  return result;
}
``

divides its execution time by 3.

And the total time of kinto drops to ~230ms: https://perfht.ml/2zaLIVQ

Patch will come :)
143ms after disabling useless sorting of records :)
https://perfht.ml/2OJKyWj

I believe I'm done for real now.
r=me with some minor nits on the current version of the patch. Thanks for working on this, Mathieu!

So this is a really good start, but we should probably investigate 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. Mathieu, can you file a follow-up for that? Thank you!
Flags: needinfo?(mathieu)
Blocks: 1508332
Attachment #9020783 - Attachment description: Bug 1502146 - Prevent polling to run from the main thread r?Gijs → Bug 1502146 - Reduce impact of RemoteSettings synchronization on main tread r=Gijs
Pushed by mleplatre@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81abb539b39c
Reduce impact of RemoteSettings synchronization on main tread r=Gijs
Backed out for failing xpcshell at services/settings/test/unit/test_remote_settings_worker.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=81abb539b39c8d2f483d18234bd6014d6e521fc6

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=212740224&repo=autoland&lineNumber=1673

[task 2018-11-19T23:31:49.058Z] 23:31:49     INFO -  TEST-START | services/settings/test/unit/test_remote_settings_poll.js
[task 2018-11-19T23:32:01.600Z] 23:32:01     INFO -  TEST-PASS | services/settings/test/unit/test_remote_settings_poll.js | took 12542ms
[task 2018-11-19T23:32:02.221Z] 23:32:02     INFO -  adb Ignoring attempt to chmod external storage
[task 2018-11-19T23:32:02.839Z] 23:32:02     INFO -  adb Ignoring attempt to chmod external storage
[task 2018-11-19T23:32:03.152Z] 23:32:03     INFO -  adb Ignoring attempt to chmod external storage
[task 2018-11-19T23:32:03.356Z] 23:32:03     INFO -  adb Ignoring attempt to chmod external storage
[task 2018-11-19T23:32:03.358Z] 23:32:03     INFO -  TEST-START | services/settings/test/unit/test_remote_settings_worker.js
[task 2018-11-19T23:32:08.178Z] 23:32:08  WARNING -  TEST-UNEXPECTED-FAIL | services/settings/test/unit/test_remote_settings_worker.js | xpcshell return code: 0
[task 2018-11-19T23:32:08.179Z] 23:32:08     INFO -  TEST-INFO took 4820ms
[task 2018-11-19T23:32:08.179Z] 23:32:08     INFO -  >>>>>>>
[task 2018-11-19T23:32:08.179Z] 23:32:08     INFO -  services/settings/test/unit/test_remote_settings_worker.js | xpcw: cd /sdcard/tests/xpc/services/settings/test/unit
[task 2018-11-19T23:32:08.179Z] 23:32:08     INFO -  services/settings/test/unit/test_remote_settings_worker.js | xpcw: xpcshell -r /sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/sdcard/tests/xpc/p/mozinfo.json"; -e const _PREFS_FILE = "/sdcard/tests/xpc/user.js"; -e const _TESTING_MODULES_DIR = "/sdcard/tests/xpc/m"; -f /sdcard/tests/xpc/head.js -e const _HEAD_FILES = ["/sdcard/tests/xpc/services/settings/test/unit/../../../common/tests/unit/head_global.js", "/sdcard/tests/xpc/services/settings/test/unit/../../../common/tests/unit/head_helpers.js"]; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_remote_settings_worker.js"]; -e const _TEST_NAME = "services/settings/test/unit/test_remote_settings_worker.js"; -e _execute_test(); quit(0);
[task 2018-11-19T23:32:08.180Z] 23:32:08     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-11-19T23:32:08.180Z] 23:32:08     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2018-11-19T23:32:08.180Z] 23:32:08     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-11-19T23:32:08.181Z] 23:32:08     INFO -  running event loop
[task 2018-11-19T23:32:08.181Z] 23:32:08     INFO -  services/settings/test/unit/test_remote_settings_worker.js | Starting test_canonicaljson_merges_remote_into_local
[task 2018-11-19T23:32:08.182Z] 23:32:08     INFO -  (xpcshell/head.js) | test test_canonicaljson_merges_remote_into_local pending (2)
[task 2018-11-19T23:32:08.182Z] 23:32:08     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2018-11-19T23:32:08.183Z] 23:32:08     INFO -  TEST-PASS | services/settings/test/unit/test_remote_settings_worker.js | test_canonicaljson_merges_remote_into_local - [test_canonicaljson_merges_remote_into_local : 16] "{\\"data\\":[{\\"id\\":\\"1\\",\\"title\\":\\"title 1\\"},{\\"id\\":\\"2\\",\\"title\\":\\"title b\\"}],\\"last_modified\\":\\"42\\"}" == "{\\"data\\":[{\\"id\\":\\"1\\",\\"title\\":\\"title 1\\"},{\\"id\\":\\"2\\",\\"title\\":\\"title b\\"}],\\"last_modified\\":\\"42\\"}"
[task 2018-11-19T23:32:08.183Z] 23:32:08     INFO -  (xpcshell/head.js) | test run_next_test 1 pending (2)
[task 2018-11-19T23:32:08.184Z] 23:32:08     INFO -  (xpcshell/head.js) | test test_canonicaljson_merges_remote_into_local finished (2)
[task 2018-11-19T23:32:08.184Z] 23:32:08     INFO -  services/settings/test/unit/test_remote_settings_worker.js | Starting test_import_json_dump_into_idb
[task 2018-11-19T23:32:08.184Z] 23:32:08     INFO -  (xpcshell/head.js) | test test_import_json_dump_into_idb pending (2)
[task 2018-11-19T23:32:08.185Z] 23:32:08     INFO -  services/settings/test/unit/test_remote_settings_worker.js | JavaScript strict warning: resource://services-common/kinto-http-client.js, line 1796: ReferenceError: reference to undefined property "initializer"
[task 2018-11-19T23:32:08.185Z] 23:32:08     INFO -  services/settings/test/unit/test_remote_settings_worker.js | JavaScript strict warning: resource://services-common/kinto-http-client.js, line 1177: ReferenceError: reference to undefined property "initializer"
[task 2018-11-19T23:32:08.186Z] 23:32:08     INFO -  services/settings/test/unit/test_remote_settings_worker.js | JavaScript strict warning: resource://services-common/kinto-http-client.js, line 323: ReferenceError: reference to undefined property "initializer"
[task 2018-11-19T23:32:08.186Z] 23:32:08     INFO -  (xpcshell/head.js) | test run_next_test 1 finished (2)
[task 2018-11-19T23:32:08.187Z] 23:32:08     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "initializer"" {file: "resource://services-common/kinto-http-client.js" line: 1796}]"
[task 2018-11-19T23:32:08.187Z] 23:32:08     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "initializer"" {file: "resource://services-common/kinto-http-client.js" line: 1177}]"
[task 2018-11-19T23:32:08.188Z] 23:32:08     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "initializer"" {file: "resource://services-common/kinto-http-client.js" line: 323}]"
[task 2018-11-19T23:32:08.188Z] 23:32:08     INFO -  TEST-PASS | services/settings/test/unit/test_remote_settings_worker.js | test_import_json_dump_into_idb - [test_import_json_dump_into_idb : 27] 0 == 0
[task 2018-11-19T23:32:08.188Z] 23:32:08  WARNING -  TEST-UNEXPECTED-FAIL | services/settings/test/unit/test_remote_settings_worker.js | test_import_json_dump_into_idb - [test_import_json_dump_into_idb : 32] false == true

Backout: https://hg.mozilla.org/integration/autoland/rev/2b2fd36ea6c1ab124817d6826af6d38d3f1306b7
Flags: needinfo?(mathieu)
Attachment #9020783 - Attachment description: Bug 1502146 - Reduce impact of RemoteSettings synchronization on main tread r=Gijs → Bug 1502146 - Reduce impact of RemoteSettings synchronization on main thread r=Gijs
Pushed by mleplatre@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51c30c756ab0
Reduce impact of RemoteSettings synchronization on main thread r=Gijs
https://hg.mozilla.org/mozilla-central/rev/51c30c756ab0
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Quick question, Mathieu (and sorry for being late on this) - should we be leaving this worker around or cleaning it up when it's inactive? How often is it expected to be doing work?
Flags: needinfo?(mathieu)
See Also: → 1509267
Hi Doug, 
Thanks, I opened Bug 1509267
Could you please comment there to give some insights how to do this? Can the GC do it as other stuff?
Flags: needinfo?(mathieu)
Performance Impact: --- → P2
Keywords: perf:responsiveness
Whiteboard: [qf:p2:responsiveness][fxperf:p2] → [fxperf:p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: