Skip to content
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.

Sign up for GitHub

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

stream: pre-allocate _events #50428

Merged
merged 1 commit into from
Oct 29, 2023
Merged

stream: pre-allocate _events #50428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions lib/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ const {

const kCapture = Symbol('kCapture');
const kErrorMonitor = Symbol('events.errorMonitor');
const kShapeMode = Symbol('shapeMode');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this wouldn't be an extra property and share a bit field with captureRejections and maybe _maxListeners, but that doesn't need to happen in this PR

const kMaxEventTargetListeners = Symbol('events.maxEventTargetListeners');
const kMaxEventTargetListenersWarned =
Symbol('events.maxEventTargetListenersWarned');
Expand Down Expand Up @@ -344,6 +345,9 @@ EventEmitter.init = function(opts) {
this._events === ObjectGetPrototypeOf(this)._events) {
this._events = { __proto__: null };
this._eventsCount = 0;
this[kShapeMode] = false;
benjamingr marked this conversation as resolved.
Show resolved Hide resolved
} else {
this[kShapeMode] = true;
}

this._maxListeners = this._maxListeners || undefined;
Expand Down Expand Up @@ -686,9 +690,13 @@ EventEmitter.prototype.removeListener =
return this;

if (list === listener || list.listener === listener) {
if (--this._eventsCount === 0)
this._eventsCount -= 1;

if (this[kShapeMode]) {
events[type] = undefined;
} else if (this._eventsCount === 0) {
this._events = { __proto__: null };
else {
} else {
delete events[type];
if (events.removeListener)
this.emit('removeListener', type, list.listener || listener);
Expand Down Expand Up @@ -750,6 +758,7 @@ EventEmitter.prototype.removeAllListeners =
else
delete events[type];
}
this[kShapeMode] = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? What if we just have an object template (template in the abstract way not v8 one) and use that when resetting? (Same for the one below)

(This is not really important comment as it's not happening on a regular basis)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe reset _events to the template you provided (e.g. with data event key) instead to the default one (i.e. empty with proto null)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require us to keep the template in memory and then create copies. Maybe having some kind of reset/initEvents override is better but starts to make things complciated.

return this;
}

Expand All @@ -762,6 +771,7 @@ EventEmitter.prototype.removeAllListeners =
this.removeAllListeners('removeListener');
this._events = { __proto__: null };
this._eventsCount = 0;
this[kShapeMode] = false;
return this;
}

Expand Down
18 changes: 18 additions & 0 deletions lib/internal/streams/duplex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,24 @@ function Duplex(options) {
if (!(this instanceof Duplex))
return new Duplex(options);

this._events ??= {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking nit: a comment explaining the purpose of defining this like this would be helpful for future generations coming into this code :-)

rluvaton marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really think we should modify the private property _events like that, maybe have an option or an api for setting events template in the EventEmitter class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up PR welcome?

close: undefined,
error: undefined,
prefinish: undefined,
finish: undefined,
drain: undefined,
data: undefined,
end: undefined,
readable: undefined,
// Skip uncommon events...
// pause: undefined,
// resume: undefined,
// pipe: undefined,
// unpipe: undefined,
// [destroyImpl.kConstruct]: undefined,
// [destroyImpl.kDestroy]: undefined,
};

this._readableState = new Readable.ReadableState(options, this, true);
this._writableState = new Writable.WritableState(options, this, true);

Expand Down
15 changes: 15 additions & 0 deletions lib/internal/streams/readable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,21 @@ function Readable(options) {
if (!(this instanceof Readable))
return new Readable(options);

this._events ??= {
ronag marked this conversation as resolved.
Show resolved Hide resolved
close: undefined,
error: undefined,
data: undefined,
end: undefined,
readable: undefined,
// Skip uncommon events...
// pause: undefined,
// resume: undefined,
// pipe: undefined,
// unpipe: undefined,
// [destroyImpl.kConstruct]: undefined,
// [destroyImpl.kDestroy]: undefined,
};

this._readableState = new ReadableState(options, this, false);

if (options) {
Expand Down
11 changes: 11 additions & 0 deletions lib/internal/streams/writable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,17 @@ function Writable(options) {
if (!(this instanceof Writable))
return new Writable(options);

this._events ??= {
close: undefined,
error: undefined,
prefinish: undefined,
finish: undefined,
drain: undefined,
// Skip uncommon events...
// [destroyImpl.kConstruct]: undefined,
// [destroyImpl.kDestroy]: undefined,
};

this._writableState = new WritableState(options, this, false);

if (options) {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-readline-interface.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class FakeInput extends EventEmitter {
function isWarned(emitter) {
for (const name in emitter) {
const listeners = emitter[name];
if (listeners.warned) return true;
if (listeners && listeners.warned) return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking nit: Wouldn't if (listeners?.warned) have the same effect here?

}
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-readline-promises-interface.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class FakeInput extends EventEmitter {
function isWarned(emitter) {
for (const name in emitter) {
const listeners = emitter[name];
if (listeners.warned) return true;
if (listeners && listeners.warned) return true;
}
return false;
}
Expand Down