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

RESOLVED FIXED in Firefox 40

Status

RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

unspecified
Firefox 40

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8599569 [details] [diff] [review]
eventemitter-pref.patch

What do you think?
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8599569 - Flags: review?(nfitzgerald)
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;
  }
Attachment #8599569 - Flags: review?(nfitzgerald) → review+
(Assignee)

Comment 3

3 years ago
Created attachment 8599589 [details] [diff] [review]
eventemitter-pref-r=fitzgen.patch

Made suggested changes.  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5a96503d095
Attachment #8599569 - Attachment is obsolete: true
Attachment #8599589 - Flags: review+
(Assignee)

Comment 4

3 years ago
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`
(Assignee)

Comment 5

3 years ago
(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
(Assignee)

Comment 6

3 years ago
Created attachment 8599876 [details] [diff] [review]
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
Attachment #8599589 - Attachment is obsolete: true
Attachment #8599876 - Flags: review+
(Assignee)

Comment 7

3 years ago
remote:   https://hg.mozilla.org/integration/fx-team/rev/84a280a8e16a
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/84a280a8e16a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.