Issue 7202 - Add hasListeners method to EventEmitter

[Manish Jethani, 2019-01-09 14:53:01]

Patch Set 1

[Manish Jethani, 2019-01-09 17:04:37]

Patch Set 2: Add hasListeners method
Patch Set 3: Optimize all methods
Patch Set 4: Add tests

I'll update the Trac issue, but the idea is that we need to have a
`hasListeners` method that checks if there are any listeners. This will allow us
to follow optimized code paths in the extension when DevTools is closed.

[hub, 2019-01-09 22:42:40]

https://codereview.adblockplus.org/29977555/diff/29977562/lib/events.js
File lib/events.js (right):

https://codereview.adblockplus.org/29977555/diff/29977562/lib/events.js#newco...
lib/events.js:38: let listeners = this._listeners.size > 0 ?
this._listeners.get(name) : null;
I don't see how this is faster (since it is part of the "optimize all methods"
change) ? And given that we only check whether listener is falsey, it shouldn't
matter whether it is undefined or null.

[Manish Jethani, 2019-01-09 23:09:45]

https://codereview.adblockplus.org/29977555/diff/29977562/lib/events.js
File lib/events.js (right):

https://codereview.adblockplus.org/29977555/diff/29977562/lib/events.js#newco...
lib/events.js:38: let listeners = this._listeners.size > 0 ?
this._listeners.get(name) : null;
On 2019/01/09 22:42:40, hub wrote:
> I don't see how this is faster (since it is part of the "optimize all methods"
> change) ? And given that we only check whether listener is falsey, it
shouldn't
> matter whether it is undefined or null.

This is not about null vs. undefined, this is about avoiding the lookup via
get() when there are no listeners for any event (as is the case with
lib/hitLogger.js in the extension most of the time because DevTools is not open
for any tab).

If you mean specifically about this instance in the on() function, then again
avoiding lookup for the first listener for an event might help.

I don't feel very strongly about on() and off() but I think it helps in the case
of emit() because an event is emitted for each filter hit.

[Manish Jethani, 2019-01-09 23:11:56]

https://codereview.adblockplus.org/29977555/diff/29977562/lib/events.js
File lib/events.js (right):

https://codereview.adblockplus.org/29977555/diff/29977562/lib/events.js#newco...
lib/events.js:38: let listeners = this._listeners.size > 0 ?
this._listeners.get(name) : null;
On 2019/01/09 23:09:45, Manish Jethani wrote:
> On 2019/01/09 22:42:40, hub wrote:
> > I don't see how this is faster (since it is part of the "optimize all
methods"
> > change) ? And given that we only check whether listener is falsey, it
> shouldn't
> > matter whether it is undefined or null.
> 
> This is not about null vs. undefined, this is about avoiding the lookup via
> get() when there are no listeners for any event (as is the case with
> lib/hitLogger.js in the extension most of the time because DevTools is not
open
> for any tab).
> 
> If you mean specifically about this instance in the on() function, then again
> avoiding lookup for the first listener for an event might help.
> 
> I don't feel very strongly about on() and off() but I think it helps in the
case
> of emit() because an event is emitted for each filter hit.

Having said this, the "optimize all methods" part is not necessary for this
change, I can drop it if you think we should do it separately.

[Manish Jethani, 2019-01-10 03:50:13]

https://codereview.adblockplus.org/29977555/diff/29977562/lib/events.js
File lib/events.js (right):

https://codereview.adblockplus.org/29977555/diff/29977562/lib/events.js#newco...
lib/events.js:38: let listeners = this._listeners.size > 0 ?
this._listeners.get(name) : null;
On 2019/01/09 23:09:45, Manish Jethani wrote:
> On 2019/01/09 22:42:40, hub wrote:
> > I don't see how this is faster (since it is part of the "optimize all
methods"
> > change) ? And given that we only check whether listener is falsey, it
> shouldn't
> > matter whether it is undefined or null.
> 
> This is not about null vs. undefined, this is about avoiding the lookup via
> get() when there are no listeners for any event (as is the case with
> lib/hitLogger.js in the extension most of the time because DevTools is not
open
> for any tab).

Alright, so I wrote this test:

  // test.js
  let {EventEmitter} = require("./lib/events");
  
  let eventEmitter = new EventEmitter();
    
  let start = Date.now();    
  for (let i = 0; i < 10000000; i++)
    eventEmitter.emit("foo");    
  console.log(Date.now() - start);

For me it's thrice as fast with the additional check for size.

I think we could skip the checks in on() and off() but keep them in listeners(),
hasListeners() and emit(). There is a cost (~10%) when the size is in fact
non-zero.

What do you think?

[Manish Jethani, 2019-01-10 04:01:43]

Patch Set 5: Remove checks from on() and off()

[hub, 2019-01-10 18:07:10]

LGTM