As per @isaacs and @ry's request, changes to pull request for events #1457

Closed
wants to merge 9 commits into
from

Projects

None yet

10 participants

@0x00A
0x00A commented Aug 4, 2011

this pull request now includes fixes from @koichik's code review.

hij1nx added some commits Aug 1, 2011
hij1nx [lib] events update, adds propper namespacing and wildcard support edfe18f
hij1nx [doc/api] updated docs to reflect the changes to events.js 189ebb5
hij1nx [lib] trailing whitespace removed, named functions, internally used f…
…unctions privatized (reduced surface area), onAny returns , [doc] lines wrapped
82397be
hij1nx [lib] emitting error should check this._all, listenersAny() implement…
…ed, 'un' changed to 'off' for API clarity, [doc/api] correction L#29, correction L#140, correction L#87, correction L#61, lines should not exceed 80 chars.
350520e
hij1nx [doc] slight restructure 23db018
hij1nx Merge https://github.com/joyent/node 1cfd6c6
Hij1nx - Nodejitsu copy error fixed. d716137
hij1nx [lib] updates 'this.event' more frequently, [doc] updates to reflect …
…'this.event'
8891219
hij1nx [lib] updates 'this.event' more frequently, [doc] updates to reflect …
…'this.event'
82ca8c8
@0x00A
0x00A commented Aug 5, 2011

@koichik thank you for spending time on this and helping to carefully review!!

@felixge
felixge commented Aug 5, 2011

I'm still -1 on merging this as a big patch like this.

@koichik
koichik commented Aug 6, 2011

@hij1nx - Nice! but I think this warning is still necessary.

     emitter.on('foo', function() {
-      console.log(this.event); // => `foo`
-      emitter.emit('bar'); // emitting a new event changes the current event.
-      console.log(this.event); // => `bar`
+      emitter.emit('bar');
     });
@polotek
polotek commented Aug 8, 2011

In an effort to refocus this discussion, I've submitted a patch to enable simple support for global listeners. #1478

No offense to @hij1nx.

@0x00A
0x00A commented Aug 8, 2011

This will not only conflict with my patch fundamentally but also my library. There are a lot of implications that are not considered here. And * is historically associated with namespace conventions. Since this intends to catch everything, then there will need to be a new semantic for single spaced namespaces. Using * in the event string suggests now that i may be able to do foo* or foo.* or perhaps (\d.*). @polotek, i believe your patch presents many potential conflicts.

@indexzero
Member

@felixge is your only gripe that it is big? It seems by that same logic you would have -1'ed the HTTP rewrite from @mikeal, yet you did not. Can you explain this discrepancy in your judgement?

@mikeal
Member
mikeal commented Aug 8, 2011

EventEmitter isn't broken, http1 was :P

but in all seriousness, no code is hit more than EventEmitter. small performance patches have made node 20% faster because the code is so hot.

there is reason to be concerned by the volume of a change.

my biggest concern, which is why i want to see it broken in to separate patches, is that we don't have any room to discuss the API and possibly make changes because the conversation is highjacked by a debate about whether the entire thing should go in or not.

@indexzero
Member

@mikeal EE2 has solid benchmarks and there is no performance degredation running node with this patch.

Yes it is a large commit.
Yes it adds several features (which are critical imho, see: https://gist.github.com/1122318)
No its not slow
No it doesn't break anything
No it isn't enabled by default

There seems to be a lot of dissent from people who are simply concerned for the sake of being concerned; to near luddite proportions. It would be sad to me if this was dead in the water because of that kind of mentality

@felixge
felixge commented Aug 9, 2011

@felixge is your only gripe that it is big? It seems by that same logic you would have -1'ed the HTTP rewrite from @mikeal, yet you did not. Can you explain this discrepancy in your judgement?

Afaik @mikael's rewrite did not introduce new API (I could be mistaken).

The reason I am -1 on this is because it adds multiple features to one of the most critical objects in the node core at once. I am all for evolving the EventEmitter, but this is too much, too fast. It's basically impossible for me to follow the whole discussion on the thing because the list of sub-discussions is endless. Breaking things down into smaller patches would allow for better discussions and probably a nicer end result.

--fg

@0x00A
0x00A commented Aug 9, 2011

@ry @isaacs @felixge I am on board with the idea of breaking this up into smaller pull requests. I think I may be in the best position to do this based on the fact that i already have a ~50% performance optimization in my fork which lends itself to a ~5% overall performance increase for node (this is reduced to about 4% with the current features i've added) I've also spent hundreds of hours studying this issue. Can we start an email thread with the feature would you like to see first?

@tj
tj commented Aug 9, 2011

does anyone know know how often people use "this" within the callbacks? might be too micro to consider (I didn't test in EE itself) but below are the consistent results I get for a call with no receiver vs .call(). I didn't even know "this" in those callbacks was significant so if no one else is really using it then it might be worth changing.

no receiver : 34721059 ops/s
       call : 29046426 ops/s
@indexzero
Member

@visionmedia That's a good question. If this is not used frequently enough in EE callbacks it could be a meaningful breaking change for future EE2 changes. As a couple of people have pointed out in EE2 this.event can be unreliable:

emitter.on('foo', function() {
  console.log(this.event); // => `foo`
  emitter.emit('bar'); // emitting a new event changes the current event.
  console.log(this.event); // => `bar`
});

If we make this breaking change, that wouldn't be the case anymore because we could bind to an object literal, { event: 'eventName' }

@mikeal
Member
mikeal commented Aug 9, 2011

The http client rewrite did change the Agent API and in fact is a reverse incompatible change to the Agent API.

Most people don't use the Agent API directly so it's pretty low impact, but it is a breaking change.

@mikeal
Member
mikeal commented Aug 9, 2011

can we add a test to the benchmark that shows instantiation time. it seems like we're only testing emits and I'm concerned that instantiation time could grow a little too much and we create a lot of these objects :)

@0x00A
0x00A commented Aug 9, 2011

@mikeal which test are we talking about?

@polotek
polotek commented Aug 9, 2011

I was looking at this last night and changed the benchmarks.js file in EE2 a bit.

https://gist.github.com/1134827

It isolates the different functions a little better. With this test it looks like instantiation does take a bit hit because the constructor now does work. However, the performance improvements to dispatch in @hij1nx's patch are really impressive. We definitely want to take those.

FYI, this is my first time using benchmark.js. Good stuff. But let me know if I'm doing something wrong here.

@0x00A
0x00A commented Aug 9, 2011

@polotek, im working on a patch witch is a combination of mine and yours. As i said before, my primary concerns with your patch were performance penalty and the lack of visibility into the actual event fired. I believe i have both of these issues resolved.

IIRC, the ctor is not required to execute. Please observe this micro-optimization found in several functions...

    this._events || init.call(this);

alot of the core codebase assumes an empty ctor or in a lot of cases completely disregards it; the above code accounts for that.

In regards to benchmarkjs it looks like you know what you're doing ;)

@polotek
polotek commented Aug 9, 2011

@hij1nx Yeah I saw that, but just realized that my test isn't really representative of usage. I'm creating emitters with the base constructor, but it's almost always subclassed right?

@0x00A
0x00A commented Aug 9, 2011

Its frequently subclassed in core. Yet it is a 1:1 loss, excersized at different junctures.

@3rd-Eden 3rd-Eden commented on the diff Aug 13, 2011
lib/events.js
+ }
+ else if(typeof tree._listeners === 'function') {
+ tree._listeners = [tree._listeners, listener];
+ }
+ else if (isArray(tree._listeners)) {
+
+ tree._listeners.push(listener);
+
+ if (!tree._listeners.warned) {
+
+ var m = defaultMaxListeners;
+
+ if (m > 0 && tree._listeners.length > m) {
+
+ tree._listeners.warned = true;
+ console.error('(node) warning: possible EventEmitter memory ' +
@3rd-Eden
3rd-Eden Aug 13, 2011

I know it's a legacy code, but shouldn't this be console.warn instead of console.error as it outputs a warning and not a 'real' error?

@mikeal
Member
mikeal commented Aug 15, 2011

lately i've been seeing pipe() as a definition of flow control more than specifically about pushing events from one place to another. you can see this in the latest work i've done on request.

but, that doesn't mean that node-core should implement a new style of pipe(). I actually depend on core's pipe doing a great job of pushing data and handling push back in the pipe() that is beneath my domain specific pipe() logic.

i don't want to increase the surface area of core's pipe(), we should leave propogating additional events in the application layer above core. if in a year or so we have some solid examples then we can talk about adding something to core, but not before that.

@indexzero
Member

@mikeal I agree we should be judicious in how we push forward with this kind of "mad science". I'll try to whip up some more meaningful examples when I get some spare cycles.

I still think having ChildProcess instances inheriting from EventEmitter2 would be useful based on this example: https://gist.github.com/1122318

@trevnorris
Collaborator

Quite a bit of discussion between this and #1478, just to drop off the face of the earth a year ago.

Just checked against current master (11a5119) and don't come close to merge-able. Will this code be fixed, or should it be closed?

@bnoordhuis
Member

Just checked against current master (11a5119) and don't come close to merge-able. Will this code be fixed, or should it be closed?

I'm closing it. This thread shows it was a controversial feature back in the day but people haven't spoken up since.

@bnoordhuis bnoordhuis closed this Nov 24, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment