-
Notifications
You must be signed in to change notification settings - Fork 30.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Performance Enhancement in EventEmitter’s emit Method #53056
Comments
If you're confident in these changes, I'd highly recommend submitting a PR - easier and more in-line with our workflow to do code review that way 👍🏻 |
The listener cloning is important as a listener could within itself remove listeners that already triggered, and indeed "once" does this, which would result in array positions changing and still registered listeners could get skipped due to shifting past the position of the index into the array during the loop. |
To address this, I propose a conditional cloning approach. The listener array will only be cloned if a change (such as removal of the listener) is detected during event execution. In this way, can we avoid unnecessary cloning and still handle edge cases correctly I wonder if this approach will be performant |
Will it be still performant if there is only one event listener? |
Sorry for the late reply, my aim was to provide conditions and reduce the cost |
Here is my suggestion: EventEmitter.prototype.emit = function emit(type, ...args) {
let doError = (type === 'error');
const events = this._events;
if (doError && events?.[kErrorMonitor] !== undefined)
this.emit(kErrorMonitor, ...args);
const handler = events?.[type];
if (handler !== undefined) {
if (typeof handler === 'function') {
const result = handler.apply(this, args);
// We check if result is undefined first because that
// is the most common case so we do not pay any perf
// penalty
if (result !== undefined && result !== null) {
addCatch(this, result, type, args);
}
} else {
const len = handler.length;
handler.isHandling = true;
for (let i = 0; i < len; ++i) {
const result = handler[i].apply(this, args);
// We check if result is undefined first because that
// is the most common case so we do not pay any perf
// penalty.
// This code is duplicated because extracting it away
// would make it non-inlineable.
if (result !== undefined && result !== null) {
addCatch(this, result, type, args);
}
}
handler.isHandling = false;
}
} else if (doError)
_handleUncaughtError(this, args)
else
return false;
};
function _handleUncaughtError(target, args) {
let er;
if (args.length > 0)
er = args[0];
if (er instanceof Error) {
try {
const capture = {};
ErrorCaptureStackTrace(capture, EventEmitter.prototype.emit);
ObjectDefineProperty(er, kEnhanceStackBeforeInspector, {
__proto__: null,
value: FunctionPrototypeBind(enhanceStackTrace, target, er, capture),
configurable: true,
});
} catch {
// Continue regardless of error.
}
// Note: The comments on the `throw` lines are intentional, they show
// up in Node's output if this results in an unhandled exception.
throw er; // Unhandled 'error' event
}
let stringifiedEr;
try {
stringifiedEr = inspect(er);
} catch {
stringifiedEr = er;
}
// At least give some kind of context to the user
const err = new ERR_UNHANDLED_ERROR(stringifiedEr);
err.context = er;
throw err; // Unhandled 'error' event
}
function _addListener(target, type, listener, prepend) {
let max;
let events;
let existing;
checkListener(listener);
events = target._events;
if (events === undefined) {
events = target._events = { __proto__: null };
target._eventsCount = 0;
} else {
// To avoid recursion in the case that type === "newListener"! Before
// adding it to the listeners, first emit "newListener".
if (events.newListener !== undefined) {
target.emit('newListener', type,
listener.listener ?? listener);
// Re-assign `events` because a newListener handler could have caused the
// this._events to be assigned to a new object
events = target._events;
}
existing = events[type];
}
if (existing === undefined) {
// Optimize the case of one listener. Don't need the extra array object.
events[type] = listener;
++target._eventsCount;
} else {
if (typeof existing === 'function') {
// Adding the second element, need to change to array.
existing = events[type] =
prepend ? [listener, existing] : [existing, listener];
// If we've already got an array, just append.
} else {
if (existing.isHandling === true)
events[type] = existing = arrayClone(existing);
prepend ? existing.unshift(listener) : existing.push(listener);
}
// Check for listener leak
max = _getMaxListeners(target);
if (max > 0 && existing.length > max && !existing.warned) {
existing.warned = true;
// No error code for this since it is a Warning
const w = genericNodeError(
`Possible EventEmitter memory leak detected. ${existing.length} ${String(type)} listeners ` +
`added to ${inspect(target, { depth: -1 })}. MaxListeners is ${max}. Use emitter.setMaxListeners() to increase limit`,
{ name: 'MaxListenersExceededWarning', emitter: target, type: type, count: existing.length });
process.emitWarning(w);
}
}
return target;
}
EventEmitter.prototype.removeListener =
function removeListener(type, listener) {
checkListener(listener);
const events = this._events;
if (events === undefined)
return this;
const list = events[type];
if (list === undefined)
return this;
if (list === listener || list.listener === listener) {
this._eventsCount -= 1;
if (this[kShapeMode]) {
events[type] = undefined;
} else if (this._eventsCount === 0) {
this._events = { __proto__: null };
} else {
delete events[type];
if (events.removeListener)
this.emit('removeListener', type, list.listener || listener);
}
} else if (typeof list !== 'function') {
let position = -1;
if (list.isHandling === true)
events[type] = list = arrayClone(list);
for (let i = list.length - 1; i >= 0; i--) {
if (list[i] === listener || list[i].listener === listener) {
position = i;
break;
}
}
if (position < 0)
return this;
if (position === 0)
list.shift();
else {
if (spliceOne === undefined)
spliceOne = require('internal/util').spliceOne;
spliceOne(list, position);
}
if (list.length === 1)
events[type] = list[0];
if (events.removeListener !== undefined)
this.emit('removeListener', type, listener);
}
return this;
};
function onceWrapper() {
if (!this.fired) {
this.target.removeListener(this.type, this.wrapFn);
this.fired = true;
this.wrapFn.listener = null;
this.wrapFn = null;
const result = (arguments.length === 0) ?
this.listener.call(this.target):
this.listener.apply(this.target, arguments);
this.target = null;
this.listener = null;
return result;
}
} My points:
|
it looks great to me thank you very much for your research would you like to open a mr about it ? @simonkcleung |
Sorry I am not going to open one because I am not familiar with it. |
Okay, I'll be opening a pr that does what you say |
hey @mertcanaltin , |
hello of course, you can open a pr and let us review it, please feel free |
Refactored error handling logic for clarity and efficiency. Streamlined error inspection and stringification process. Enhanced stack trace handling with minimal performance overhead. Simplified listener iteration and result handling in emit method. Fixes: nodejs#53056 Signed-off-by: KUNAL KUMAR <[email protected]>
hey @mertcanaltin can you review it ? |
What is the problem this feature will solve?
Specific problems addressed by this feature:
What is the feature you are proposing to solve the problem?
I propose a series of optimizations to the emit method in the EventEmitter class. These optimizations are designed to improve the performance and efficiency of event handling in Node.js. The proposed changes include:
Proposed Changes to emit Method:
lib/events.js
Benefits of the Proposed Feature:
These changes aim to enhance the overall performance and efficiency of the emit method in the EventEmitter class
What alternatives have you considered?
No response
The text was updated successfully, but these errors were encountered: