1159929 - Cache the pref value for devtools.dump.emit in event emitter

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Brian Grinstead [:bgrins]]

When doing some netmonitor profiling, we discovered that checking a pref on every emitted event was taking up a not-insignificant amount of time: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/event-emitter.js#150.

We should hook this up with a pref observer and cache the value.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: eventemitter-pref.patch

What do you think?

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8599569 [details] [diff] [review]
eventemitter-pref.patch

Review of attachment 8599569 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/event-emitter.js
@@ +19,5 @@
>    }
>  }).call(this, function (require, exports, module) {
>  
> +this.EventEmitter = function EventEmitter() {
> +  this.loggingEnabled = Services.prefs.getBoolPref("devtools.dump.emit");

Any reason to check the pref and register and observer for every EventEmitter, rather than doing this at the top level?

@@ +154,5 @@
>      }
>    },
>  
>    logEvent: function(aEvent, args) {
> +    let logging = isWorker ? true : this.loggingEnabled;

While you're here, and if you feel like it, I think this would be clearer if we just did:

  if (!this.loggingEnabled) {
    return;
  }

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: eventemitter-pref-r=fitzgen.patch

Made suggested changes.  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5a96503d095

Comment 4

[Brian Grinstead [:bgrins]]

Weird, getting an error on the toolkit/devtools unit tests `Services.prefs is undefined at resource://gre/modules/devtools/worker-loader.js -> resource://gre/modules/devtools/event-emitter.js:30`

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Weird, getting an error on the toolkit/devtools unit tests `Services.prefs
> is undefined at resource://gre/modules/devtools/worker-loader.js ->
> resource://gre/modules/devtools/event-emitter.js:30`

Figured it out - apparently Services doesn't mean the same thing when isWorker is set

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: eventemitter-pref-r=fitzgen.patch

This does the trick (only reading the pref in the non isWorker case).  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0f8fd81a404

Comment 7

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/84a280a8e16a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84a280a8e16a