Skip to content

lib: revert primordials in a hot path #38246

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

Closed
mcollina wants to merge 8 commits into from
Closed

lib: revert primordials in a hot path #38246

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f49ba49
Revert "stream: refactor to use more primordials"
mcollina Apr 10, 2021
f9763d8
stream: removed number primordials
mcollina Apr 13, 2021
1e0d89c
http: remove primordials from hot path
mcollina Apr 13, 2021
74d2497
lib: revert more primordials in the hot path
mcollina Apr 14, 2021
590a545
fixup
mcollina Apr 14, 2021
dbbdd59
fixhup: less primordials in hot path
mcollina Apr 15, 2021
4553526
fixup
mcollina Apr 15, 2021
e51967c
fixup: remove an always-on primordial in debuglog.
mcollina Apr 16, 2021
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
9 changes: 3 additions & 6 deletions lib/internal/streams/readable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@
'use strict';

const {
NumberIsInteger,
NumberIsNaN,
NumberParseInt,
ObjectDefineProperties,
ObjectKeys,
ObjectSetPrototypeOf,
Expand Down Expand Up @@ -379,7 +376,7 @@ function howMuchToRead(n, state) {
return 0;
if (state.objectMode)
return 1;
if (NumberIsNaN(n)) {
if (Number.isNaN(n)) {
Copy link
Member

@targos targos Apr 16, 2021

Choose a reason for hiding this comment

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

Do you have any evidence that "static" primordials can lead to performance issues? I'm okay with reverting the prototype ones but this is too much imo.

Copy link
Member

@targos targos Apr 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm asking because NumberIsNan is literally Number.isNaN (same function identity)

Copy link
Member Author

@mcollina mcollina Apr 16, 2021

Choose a reason for hiding this comment

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

I don't. The approach I used is to remove all of them from hot paths, literally with %s/primordial/replacement/g. If you want I can try adding those back in.

I'm 99.9% sure on the ones from *Prototype* are the major culprit (functions and arrays specifically).

// Only flow one buffer at a time.
if (state.flowing && state.length)
return state.buffer.first().length;
Expand All @@ -397,8 +394,8 @@ Readable.prototype.read = function(n) {
// in this scenario, so we are doing it manually.
if (n === undefined) {
n = NaN;
} else if (!NumberIsInteger(n)) {
n = NumberParseInt(n, 10);
} else if (!Number.isInteger(n)) {
n = Number.parseInt(n, 10);
}
const state = this._readableState;
const nOrig = n;
Expand Down