1755682 - EventEmitter#emit could be faster

Status: RESOLVED FIXED

(DevTools :: Framework, task)

Description

[Nicolas Chevobbe [:nchevobbe]]

In Bug 1755645, we noticed that emitting was slow (in https://share.firefox.dev/3gRvO8i it's been called as part of unmanage, but that should be valid for all cases.)

The profile seems to say there is no listeners. In such scenario emit should be super cheap to run. But it may have some unnecessary computations.

• in emit, we might create unexpected arrays there because of spread operators:

devtools/shared/event-emitter.js#189-191

static emit(target, type, ...rest) {
  EventEmitter._emit(target, type, false, ...rest);
}

See https://share.firefox.dev/3h6v6UR
The last line, taking 4 samples, has js::NewDenseFullyAllocatedArray
Ideally, this function shouldn't allocate anything.

• In the profile, we can see that lots of time in spent doing name lookups in this emit method

We might double the cost of the name lookups because of this code devtools/shared/event-emitter.js#267-274

// Backward compatibility with the SDK event-emitter: support wildcard listeners that
// will be called for any event. The arguments passed to the listener are the event
// type followed by the actual arguments.
// !!! This API will be removed by Bug 1391261.
const hasWildcardListeners = target[eventListeners].has("*");
if (type !== "*" && hasWildcardListeners) {
  EventEmitter.emit(target, "*", type, ...rest);
}

which is rarely used:
https://searchfox.org/mozilla-central/search?q=.on%28%22*&path=&case=true&regexp=false
While it might add an overhead in every single call of emit.

• Similarly watchFronts isn't used that much

https://searchfox.org/mozilla-central/search?q=.watchFronts%28&path=&case=true&regexp=false
And because emit isn't so cheap, it adds an overhead to every single front being managed.
We might introduce a boolean to avoid calling emit unless watchFronts is at least called one (really simple thing),
or try to get rid of watchFronts. It might be now redundant with onTargetAvailable/onTargetDestroyed (not 100% sure).

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

• wildcard support will be removed in Bug 1391261
• not using the spread operator for emit would mean reviewing our usage of it with multiple params. From quick testing (logging when emit is called with more than 2 params), there are several callsites.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1755682 - [devtools] Avoid using spread/rest operators when not necessary in EventEmitter. r=ochameau.

Depends on D139017

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1755682 - [devtools] Avoid unnecessary function call and property access in EventEmitter._emit. r=ochameau.

Some profile were showing property access, so let's see if putting target[eventListeners]
in a variable is making things better.
Also, don't call logEvent when there's no need to. Even if the function has an
early exit when it won't do anything, there's still a cost to the function call.
In this patch, we're also shifting a few things around so we can bail out as
early as we can without doing anything we wouldn't need.

Depends on D139018

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea9b001024b4
[devtools] Avoid using spread/rest operators when not necessary in EventEmitter. r=ochameau.
https://hg.mozilla.org/integration/autoland/rev/23245e36a718
[devtools] Avoid unnecessary function call and property access in  EventEmitter._emit. r=ochameau.

Comment 5

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/ea9b001024b4
https://hg.mozilla.org/mozilla-central/rev/23245e36a718