Optional third parameter to EventEmitter.addListener to specify scope

[Nikhil Marathe]

Hi,

This is a small change to EventEmitter.addListener() to allow
specifying the 'this' reference for the listener function.
It is inspired by similar functions in ExtJS and Qooxdoo, it makes it
really convenient to change the 'owner' of the listener
leading to simpler code.

For example:

var sys = require('sys')
process.on('exit', function() { this.debug("EXITED THIS IS SYS"); }, sys);

Regards,
Nikhil

[Oleg Slobodskoi]

there is an ecma5 "bind" method for that purpose.

>  0001-Adds-an-additional-parameter-to-EventEmitter.addList.patch
> 4KViewDownload

[Nikhil Marathe]

Updated to use bind and fix a small typo

I still believe allowing it as a third parameter makes it 'look
better' than using bind.
Of course this is highly subjective, if most people have an opinion
that Object.bind() should be explicitly used, then you may reject this
patch :)

Nikhil

> --
> You received this message because you are subscribed to the Google Groups "nodejs" group.
> To post to this group, send email to nod...@googlegroups.com.
> To unsubscribe from this group, send email to nodejs+un...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/nodejs?hl=en.
>
>

--
Support open formats.
Don't send me files in proprietary formats like .doc, .xls, .ppt.

[Oleg Slobodskoi]

using third parameter makes our code shorter, but it looks more or
less like a magic parameter, which is not really explicit and is not
conform with common browser event model. So I think the most will
refuse it.

> > For more options, visit this group athttp://groups.google.com/group/nodejs?hl=en.

>
> --
> Support open formats.
> Don't send me files in proprietary formats like .doc, .xls, .ppt.
>

>  0001-Adds-an-additional-parameter-to-EventEmitter.addList.patch
> 3KViewDownload

[Oleg Slobodskoi]

using third parameter makes our code shorter, but it looks more or
less like a magic parameter, which is not really explicit and is not
conform with common browser event model. So I think the most will
refuse it.

On Aug 31, 12:01 pm, Nikhil Marathe <nsm.nik...@gmail.com> wrote:

> > For more options, visit this group athttp://groups.google.com/group/nodejs?hl=en.

>
> --
> Support open formats.
> Don't send me files in proprietary formats like .doc, .xls, .ppt.
>

>  0001-Adds-an-additional-parameter-to-EventEmitter.addList.patch
> 3KViewDownload

[Jorge]

On 31/08/2010, at 11:43, Nikhil Marathe wrote:

+process.EventEmitter.prototype.addListener = function (type, listener, scope) {
if ('function' !== typeof listener) {
throw new Error('addListener only takes instances of Function');
}

if (!this._events) this._events = {};

+ var theListener = listener;
+ if (scope) {
+ theListener = function() {
+ listener.apply(scope, arguments);
+ }
+ }

Hi Nikhil,

I'd just like to point out that you may want to call it (the 3rd argument) 'that' or 'thisBinding' or 'boundThis' instead of 'scope' because 'this' is !== 'scope'. There's 3 distinct components in a JavaScript function's context: its *scope*, the bound 'this' and its VariableEnvironment. And there's no way to alter the scope once the function has been created (it's lexical).

Currently there's lots of people using it wrong, soo many that I'm afraid it may end up sticking. But -believe me- it's not correct.

Please forgive me for nitpicking :-)
Cheers,
--
Jorge.

[Oleg Slobodskoi]

correct!

scope can't be changed!

what we can change is "context".


People don't see the difference between context and scope :(

[Nikhil Marathe]

Yes,
I'm sorry about calling it 'scope', I realised that after you pointed
it out. I know what scope is, but apparently I was thinking of
something else while writing the patch. I've changed it to 'context'

Nikhil

> --
> You received this message because you are subscribed to the Google Groups "nodejs" group.
> To post to this group, send email to nod...@googlegroups.com.
> To unsubscribe from this group, send email to nodejs+un...@googlegroups.com.

> For more options, visit this group at http://groups.google.com/group/nodejs?hl=en.

[Jorge]

On 31/08/2010, at 12:01, Nikhil Marathe wrote:

> Updated to use bind and fix a small typo
>
> I still believe allowing it as a third parameter makes it 'look
> better' than using bind.
> Of course this is highly subjective, if most people have an opinion
> that Object.bind() should be explicitly used, then you may reject this
> patch :)

You've got a typo:

+ var listener = listener;

My opinion wrt this patch is: if you want to bind some 'this', it may be preferably (and it's faster) to pass it pre-bound because If not, with this patch, there's going to be zillions of gratuitous "if (scope)" executed daily, and that's going to worsen the global warming very much :-)
--
Jorge.

[Jorge]

On 31/08/2010, at 13:27, Nikhil Marathe wrote:

> Yes,
> I'm sorry about calling it 'scope', I realised that after you pointed
> it out. I know what scope is, but apparently I was thinking of
> something else while writing the patch. I've changed it to 'context'

But the object you'd pass in there is not -and there's no way for it to be- a 'context', you're passing the object to be bound to 'this', which is just a small bit of the context.
--
Jorge.

[Aaron Heckmann]

-1 Use fn.bind() instead

--
You received this message because you are subscribed to the Google Groups "nodejs" group.
To post to this group, send email to nod...@googlegroups.com.
To unsubscribe from this group, send email to nodejs+un...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/nodejs?hl=en.

--
Aaron
http://clickdummy.net

[Camilo Aguilar]

+1 for fn.bind(). It's standard.

[Isaac Schlueter]

Yes, fn.bind() is standard, but it's also standard to pass a thisp
argument to Array#map/reduce/forEach. I don't see any conceptual
friction there.

It should be about a 3 line patch, though:

-process.EventEmitter.prototype.addListener = function (type, listener) {
+process.EventEmitter.prototype.addListener = function (type, listener, thisp) {

if ('function' !== typeof listener) {
throw new Error('addListener only takes instances of Function');
}

if (!this._events) this._events = {};

+ thisp = thisp || this;
+ listener = listener.bind(thisp);
+

But it should be benched thoroughly to see if there are any
performance implications. That is VERY hot code. Even a 1-2%
slowdown there is probably unacceptable.

--i

[Jorge]

On 31/08/2010, at 19:39, Isaac Schlueter wrote:

> Yes, fn.bind() is standard, but it's also standard to pass a thisp
> argument to Array#map/reduce/forEach. I don't see any conceptual
> friction there.
>
> It should be about a 3 line patch, though:
>
> -process.EventEmitter.prototype.addListener = function (type, listener) {
> +process.EventEmitter.prototype.addListener = function (type, listener, thisp) {
> if ('function' !== typeof listener) {
> throw new Error('addListener only takes instances of Function');
> }
>
> if (!this._events) this._events = {};
>
> + thisp = thisp || this;
> + listener = listener.bind(thisp);
> +
>
> But it should be benched thoroughly to see if there are any
> performance implications. That is VERY hot code. Even a 1-2%
> slowdown there is probably unacceptable.

Even in two lines:

listener = listener.bind(thisp || this);

Glad to see you name it 'thisp' :-)
--
Jorge.

[Vitali Lovich]

addListener is hot code?  Since when?  It's the emit-path that's hot code.

That being said, yes, performance should always be tracked.

--

[Jorge]

On Sep 1, 1:24 am, Vitali Lovich <vlov...@gmail.com> wrote:
> addListener is hot code?  Since when?  It's the emit-path that's hot code.

I thought exactly the same when I read it !
--
Jorge.

[Isaac Schlueter]

> On Sep 1, 1:24 am, Vitali Lovich <vlov...@gmail.com> wrote:
>> addListener is hot code? Since when? It's the emit-path that's hot code.

Right. If *calling* a bound function is slower than calling a
non-bound function, then it could slow down the emit-path. That's
what I'm saying.

It may be faster to not bind the function at all, but instead to keep
track of the thisp until call-time, since fn.apply(this, arguments) is
often required anyhow, so that would become an unnecessary scope
correction when an event is emitted with more than 2 arguments.

--i

[Timothy Caswell]

Also addListener is fairly hot. Many events fire few times (error, data, end) and are set once per request or some other often time.

[Jorge]

On Sep 1, 2:49 am, Isaac Schlueter <i...@izs.me> wrote:
> > On Sep 1, 1:24 am, Vitali Lovich <vlov...@gmail.com> wrote:
> >> addListener is hot code?  Since when?  It's the emit-path that's hot code.
>
> Right.  If *calling* a bound function is slower than calling a
> non-bound function, then it could slow down the emit-path.  That's
> what I'm saying.

The calls to normal f()s are more than 3x faster than the calls to
f.bind'ed.

http://github.com/xk/nodeSnippets/blob/master/boundF_vs_unboundF.js
--
Jorge.

[Isaac Schlueter]

Yep. 5.15x faster on my machine. But it's good to see that the
builtin Function#bind is way faster than the monkeypatched version :)

http://github.com/isaacs/node-bench/blob/master/examples/bind.js

--i

[Evan Larkin]

A quick test shows that the difference is more like 10 times (in the favor of unbound functions).. until you start to do anything more than calling an empty function:

http://github.com/elarkin/nodeJS-bound-vs-unbound/blob/master/testBind.js

After a relatively short while, the bound and unbound versions close to within 10% of each other.

[Isaac Schlueter]

That's why microbenchmarks like these should report raw Hz as well as
relative numbers. If the values are high enough, you can safely say
it doesn't matter, no matter how big the relative difference.

The real test is seeing the effect on node's functional test benchmarks.

--i

[Marco Rogers]

I remembered a previous thread about Array.slice being slower in some
instances. I updated fakeBind to use a for loop and push. Brought
fakeBind back up to a more reasonable level I think.

Function.prototype.fakeBind = function (o) {
var fn = this
, args0 = [];
for(var i=0, len=arguments.length;i<len;i++) {
args0.push(arguments[i]);
}
return function () {
var args = [];
for(var i=0, len=arguments.length;i<len;i++) {
args.push(arguments[i]);
}
return fn.apply(o, args0.concat(args));
}
}


unbound
Raw:
> 97448.55144855144
> 97757.24275724276
> 98378.62137862138
> 98119.88011988012
> 98459.54045954045
> 96778.22177822178
> 97575.42457542458
> 96802.1978021978
> 98507.49250749251
> 96403.5964035964
> 98394.6053946054
> 97582.41758241758
> 97901.0989010989
> 97692.30769230769
> 92762.23776223777
> 97549.45054945054
> 93782.21778221778
> 98496.5034965035
> 98054.94505494506
> 98502.4975024975
> 98358.64135864135
Average (mean) 97395.60439560439

realBind
Raw:
> 17553.446553446553
> 17431.56843156843
> 17437.56243756244
> 17417.582417582416
> 17357.64235764236
> 17414.585414585414
> 17326.673326673328
> 17204.795204795206
> 17451.548451548453
> 17461.53846153846
> 17324.675324675325
> 17505.494505494506
> 17452.547452547453
> 17419.58041958042
> 17520.47952047952
> 17268.73126873127
> 17376.623376623378
> 17483.516483516483
> 17519.48051948052
> 17036.963036963036
> 17432.567432567434
Average (mean) 17399.885828457256

fakeBind
Raw:
> 7140.859140859141
> 7188.811188811189
> 7130.8691308691305
> 7207.792207792208
> 7198.801198801199
> 7199.8001998002
> 7197.802197802198
> 7037.962037962038
> 7151.848151848152
> 7084.915084915085
> 7157.842157842158
> 7173.8261738261735
> 7204.795204795205
> 7177.822177822178
> 7013.986013986014
> 7099.9000999001
> 7174.825174825175
> 6977.022977022977
> 7140.859140859141
> 7103.896103896104
> 7170.829170829171
Average (mean) 7139.764996907856

Winner: unbound
Compared with next highest (realBind), it's:
82.13% faster
5.6 times as fast
0 order(s) of magnitude faster

Compared with the slowest (fakeBind), it's:
92.67% faster
13.64 times as fast
1 order(s) of magnitude faster


Here's the other thread. It's pretty long.

http://groups.google.com/group/nodejs/browse_thread/thread/aee876d0580368a0/d3e26dfb11f93b06

I still don't think I really got why slice was getting creamed in
those benchmarks. And Erik Corry chimed in later with a modified test
that seemed to refute the findings. But here they are again.

On Sep 1, 3:14 am, Isaac Schlueter <i...@izs.me> wrote:
> That's why microbenchmarks like these should report raw Hz as well as
> relative numbers.  If the values are high enough, you can safely say
> it doesn't matter, no matter how big the relative difference.
>
> The real test is seeing the effect on node's functional test benchmarks.
>
> --i
>
>
>
> On Tue, Aug 31, 2010 at 20:08, Evan Larkin <evan.larkin....@gmail.com> wrote:
> > A quick test shows that the difference is more like 10 times (in the favor
> > of unbound functions).. until you start to do anything more than calling an
> > empty function:

> >http://github.com/elarkin/nodeJS-bound-vs-unbound/blob/master/testBin...

[Fabian Jakobs]

This patch will break removeListener. You store the bound function,
which is not accessible to the calling code. You cannot call
removeListener with the unbound function since the unbound function is
not in the listeners registry. In qooxdoo we've solved this by storing
both the context and the function in the registry. removeListener then
also needs the context arguments. This complicates the registry a
little but for qooxdoo it turned out to be a good choice (much better
than all the 'var self = this' statement in the code).

Best Fabian

[Elijah Insua]

For example:

   var sys = require('sys')
   process.on('exit', function() { this.debug("EXITED THIS IS SYS"); }, sys);

I'm having a hard time seeing why this is helpful.. It definitely seems more 'magical' but that just adds to the cognitive overhead.

I don't think this should go in.

-- Elijah 

[Isaac Schlueter]

Well, sure, that example is silly. But what about something more like:

process.on("exit", obj.logExit, obj)

Still yeah, not worth doing. Just use Function#bind if necessary.

--i

[Nikhil Marathe]

Hi,

Thanks a lot for all the feedback, especially considering the
efficiency constraints and so on.
In retrospect, perhaps this patch is unnecessary, and as Fabian points
out, qooxdoo handles it more elegantly using the registry concept. So
yes, this shouldn't go in.

On a related note, how much does the need to keep around environment
contexts when javascript closure's are used so frequently in node and
refer to so many bound variables outside of the function's lexical
scope affect the memory and time benchmarks. What I mean is that how
slow is a 'normal' function compared to a closure referring to
external entities.

Nikhil

> --
> You received this message because you are subscribed to the Google Groups "nodejs" group.
> To post to this group, send email to nod...@googlegroups.com.
> To unsubscribe from this group, send email to nodejs+un...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/nodejs?hl=en.
>
>

--

[Evan Larkin]

Closures are a little slower, but not by much. On my local machine a function referencing a variable in a closure takes 1/6th longer to execute:

http://github.com/elarkin/nodeJS-closures-vs-non-closured-functions/blob/master/closure_tests.js

Essentially, the more levels of scoping that you have to look through to find your variable, the more expensive it is to access.

[Jorge]

On 03/09/2010, at 04:16, Evan Larkin wrote:

> Closures are a little slower, but not by much. On my local machine a function referencing a variable in a closure takes 1/6th longer to execute:
>
> http://github.com/elarkin/nodeJS-closures-vs-non-closured-functions/blob/master/closure_tests.js
>
> Essentially, the more levels of scoping that you have to look through to find your variable, the more expensive it is to access.

Exactly. Except for the global context which seems to be an optimized case.

Here's some actual numbers, on a Mac:

1938 ms 9.69 ns 103.20 Mhz ctxGlobal
3848 ms 19.24 ns 51.98 Mhz ctx9
3424 ms 17.12 ns 58.41 Mhz ctx8
3286 ms 16.43 ns 60.86 Mhz ctx7
2920 ms 14.60 ns 68.49 Mhz ctx6
2458 ms 12.29 ns 81.37 Mhz ctx5
2117 ms 10.59 ns 94.47 Mhz ctx4
1861 ms 9.30 ns 107.47 Mhz ctx3
1627 ms 8.13 ns 122.93 Mhz ctx2
1497 ms 7.49 ns 133.60 Mhz ctx1
1440 ms 7.20 ns 138.89 Mhz ctx0

http://github.com/xk/nodeSnippets/blob/68d62ec7e0cf77542d5c48092b5e970dd0a8bf1e/closureVarsAccessTimes.js

global.ctxGlobal= "ctxGlobal";
var ctx9= "ctx9";
(function () {
var ctx8= "ctx8";
(function () {
var ctx7= "ctx7";
(function () {
var ctx6= "ctx6";
(function () {
var ctx5= "ctx5";
(function () {
var ctx4= "ctx4";
(function () {
var ctx3= "ctx3";
(function () {
var ctx2= "ctx2";
(function () {
var ctx1= "ctx1";
(function () {
var ctx0= "ctx0";
get.ctx0= function () { return ctx0 };
get.ctx1= function () { return ctx1 };
get.ctx2= function () { return ctx2 };
get.ctx3= function () { return ctx3 };
get.ctx4= function () { return ctx4 };
get.ctx5= function () { return ctx5 };
get.ctx6= function () { return ctx6 };
get.ctx7= function () { return ctx7 };
get.ctx8= function () { return ctx8 };
get.ctx9= function () { return ctx9 };
get.ctxGlobal= function () { return ctxGlobal };
})();
})();
})();
})();
})();
})();
})();
})();
})();

--
Jorge.