-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
lib: fix expensive isNaN call in readable streams #1925
Conversation
Replace a call to `isNaN(n)` with `n !== n`. Removes most of the overhead from the following hotspot: 13.59% iojs perf-603.map [.] LazyCompile:*isNaN native v8natives.js:67
LGTM |
ref: #936 |
LGTM |
LGTM but perhaps there should be a comment in case someone stumbles upon this and does not understand it? |
@@ -211,7 +211,7 @@ function howMuchToRead(n, state) { | |||
if (state.objectMode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if state
is undefined
?
@bnoordhuis What would you like to do here? |
A note: we're really checking for |
@chrisdickinson if you're sure that's what it is then it sounds good to me.
|
Perhaps the original isn't an issue anymore with a newer v8?
From |
@Fishrock123 You're not passing a request path. Try |
This should really just be We are not checking types anywhere else, as can be seen by the possible type explosion: |
@bnoordhuis what is the status of this one liner? |
Closing, I'll revisit when it comes up again. |
Replace a call to
isNaN(n)
withn !== n
. Removes most of theoverhead from the following hotspot:
R=@chrisdickinson, /cc @trevnorris
CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/784/