Noissue - Make _listeners a Set object

Use a Set object instead of an array.

[Manish Jethani, 2017-04-12 12:21:09]

Patch Set 1

Use a Set object instead of an array.

[Sebastian Noack, 2017-04-12 12:32:14]

https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js#newco...
ext/common.js:40: let listeners = [...this._listeners];
Is turning the set into an array even necessary, can't you iterate over the set
directly below?

[Manish Jethani, 2017-04-12 12:37:50]

https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js#newco...
ext/common.js:40: let listeners = [...this._listeners];
On 2017/04/12 12:32:14, Sebastian Noack wrote:
> Is turning the set into an array even necessary, can't you iterate over the
set
> directly below?

Yeah, I'm trying to be compatible with the current implementation. If a value is
removed from the set while you're iterating over it, it'll be skipped. It seems
we want to call all the listeners in the current iteration, even if the set is
modified.

https://hg.adblockplus.org/adblockpluschrome/rev/f81e41006b94

[Sebastian Noack, 2017-04-12 12:42:36]

LGTM

https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js#newco...
ext/common.js:40: let listeners = [...this._listeners];
On 2017/04/12 12:37:50, Manish Jethani wrote:
> On 2017/04/12 12:32:14, Sebastian Noack wrote:
> > Is turning the set into an array even necessary, can't you iterate over the
> set
> > directly below?
> 
> Yeah, I'm trying to be compatible with the current implementation. If a value
is
> removed from the set while you're iterating over it, it'll be skipped. It
seems
> we want to call all the listeners in the current iteration, even if the set is
> modified.
> 
> https://hg.adblockplus.org/adblockpluschrome/rev/f81e41006b94

I see. This might have been the reason why the list of listeners has been copied
in the first place.

[Manish Jethani, 2017-04-12 13:38:53]

Merged.

https://hg.adblockplus.org/adblockpluschrome/rev/396ad2a01718

[Wladimir Palant, 2017-04-12 14:43:03]

https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js#newco...
ext/common.js:40: let listeners = [...this._listeners];
On 2017/04/12 12:42:36, Sebastian Noack wrote:
> I see. This might have been the reason why the list of listeners has been
copied
> in the first place.

It is. However, is that an efficient way of copying a set? I can imagine new
Set(this._listeners) being considerably better.

[Sebastian Noack, 2017-04-12 14:49:30]

https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js#newco...
ext/common.js:40: let listeners = [...this._listeners];
On 2017/04/12 14:43:02, Wladimir Palant wrote:
> On 2017/04/12 12:42:36, Sebastian Noack wrote:
> > I see. This might have been the reason why the list of listeners has been
> copied
> > in the first place.
> 
> It is. However, is that an efficient way of copying a set? I can imagine new
> Set(this._listeners) being considerably better.

We don't need a set, an array is sufficient. So if [...] has performance issues
(I wouldn't know why it should), we should rather use Array.from(), I guess.

[Manish Jethani, 2017-04-12 15:24:48]

On 2017/04/12 14:43:03, Wladimir Palant wrote:
> https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js
> File ext/common.js (right):
> 
>
https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js#newco...
> ext/common.js:40: let listeners = [...this._listeners];
> On 2017/04/12 12:42:36, Sebastian Noack wrote:
> > I see. This might have been the reason why the list of listeners has been
> copied
> > in the first place.
> 
> It is. However, is that an efficient way of copying a set? I can imagine new
> Set(this._listeners) being considerably better.

I just tested this on Node.js, Chrome, and Firefox, and the array approach is
significantly faster. I can see how this could be optimized, but it isn't. Using
a new set just adds additional overhead.

[Manish Jethani, 2017-04-12 15:42:49]

On 2017/04/12 15:24:48, Manish Jethani wrote:

> I just tested this on Node.js, Chrome, and Firefox, and the array approach is
> significantly faster. I can see how this could be optimized, but it isn't.
Using
> a new set just adds additional overhead.

Having said this, maybe we should reconsider why we're making a copy in the
first place. A set can be modified on the fly while iterating over it. It means
it should be safe to remove any listeners whether they have been called or not.

[Sebastian Noack, 2017-04-12 17:25:16]

On 2017/04/12 15:42:49, Manish Jethani wrote:
> On 2017/04/12 15:24:48, Manish Jethani wrote:
> 
> > I just tested this on Node.js, Chrome, and Firefox, and the array approach
is
> > significantly faster. I can see how this could be optimized, but it isn't.
> Using
> > a new set just adds additional overhead.
> 
> Having said this, maybe we should reconsider why we're making a copy in the
> first place. A set can be modified on the fly while iterating over it. It
means
> it should be safe to remove any listeners whether they have been called or
not.

This is the original issue: https://issues.adblockplus.org/ticket/3427

It seems the problem back then was that a listener that removed itself caused
the next listener to be skipped. This indeed wouldn't be a problem with sets
anymore. But there would still be one scenario in which the behavior differs,
that is if one listener removes another listener that has been registered after
the former listener. When first copying the listeners, that listener would still
be called, otherwise not. I cannot think of any reason why you should have two
listeners for the same event that depend on each other. So fine with me.