Closed Bug 1159929 Opened 9 years ago Closed 9 years ago

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

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch eventemitter-pref.patch (obsolete) — Splinter Review
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+
Made suggested changes.  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5a96503d095
Attachment #8599569 - Attachment is obsolete: true
Attachment #8599589 - Flags: review+
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`
(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
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+
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
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.