streams: isNaN() in howMuchToRead() is slow

[bnoordhuis]

             6.93%
                LazyCompile:*isNaN native v8natives.js:67
                LazyCompile:*howMuchToRead _stream_readable.js:208
                LazyCompile:*Readable.read _stream_readable.js:246
                LazyCompile:*readableAddChunk _stream_readable.js:122
                LazyCompile:*onread net.js:495
                Builtin:ArgumentsAdaptorTrampoline
                Builtin:JSEntryTrampoline
                Stub:JSEntryStub
                v8::internal::Execution::Call
                v8::Function::Call
                node::AsyncWrap::MakeCallback
                node::StreamWrap::OnRead
                uv__read
                uv__stream_io
                uv__io_poll
                uv_run
                node::Start
                __libc_start_main
                0xbf4e258d4c544155

This isNaN() call frequently pops up in I/O-heavy benchmarks as the top cost center.

/cc @chrisdickinson

[chrisdickinson]

Guessing this is due to polymorphic n in read(n) calls, which may be a number or undefined. We might see if it's possible to coerce n === undefined to n = -1 and check for that value in howMuchToRead. In fact, I may just go do that now ...

[chrisdickinson]

What benchmark are you using, out of curiosity?

[mscdex]

What if you replace n === null || isNaN(n) with n == null || n !== n. That should significantly improve speed while maintaining compatibility with isNaN().

[chrisdickinson]

Hm, I tried replacing the undefined value with -1, which managed to knock howMuchToRead out of the running for net-pipe, but I wasn't able to see much of a difference perf-wise. (The reasoning behind using -1 was to see if I could make the read / howMuchToRead implementation monomorphic.)

[bnoordhuis]

What benchmark are you using, out of curiosity?

@chrisdickinson That's from perf record -c 50000 -e cpu-clock -g -i out/Release/iojs --perf_basic_prof benchmark/http_simple_auto.js -k -t 10 -c 100 -n 1000000

[chrisdickinson]

If you're interested, here's the patch I was testing:

diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js
index bf73647..2a27faf 100644
--- a/lib/_stream_readable.js
+++ b/lib/_stream_readable.js
@@ -212,7 +212,7 @@ function howMuchToRead(n, state) {
   if (state.objectMode)
     return n === 0 ? 0 : 1;

-  if (n === null || isNaN(n)) {
+  if (n === -1 || n === null || isNaN(n)) {
     // only flow one buffer at a time
     if (state.flowing && state.buffer.length)
       return state.buffer[0].length;
@@ -244,11 +244,16 @@ function howMuchToRead(n, state) {

 // you can override either this method, or the async _read(n) below.
 Readable.prototype.read = function(n) {
+  if (n === undefined) n = -1;
+  return Readable_read(this, n);
+}
+
+function Readable_read(stream, n) {
   debug('read', n);
-  var state = this._readableState;
+  var state = stream._readableState;
   var nOrig = n;

-  if (typeof n !== 'number' || n > 0)
+  if (n !== 'number' || n === -1 || n > 0)
     state.emittedReadable = false;

   // if we're doing read(0) to trigger a readable event, but we
@@ -259,9 +264,9 @@ Readable.prototype.read = function(n) {
       (state.length >= state.highWaterMark || state.ended)) {
     debug('read: emitReadable', state.length, state.ended);
     if (state.length === 0 && state.ended)
-      endReadable(this);
+      endReadable(stream);
     else
-      emitReadable(this);
+      emitReadable(stream);
     return null;
   }

@@ -270,7 +275,7 @@ Readable.prototype.read = function(n) {
   // if we've ended, and we're now clear, then finish it up.
   if (n === 0 && state.ended) {
     if (state.length === 0)
-      endReadable(this);
+      endReadable(stream);
     return null;
   }

@@ -321,7 +326,7 @@ Readable.prototype.read = function(n) {
     if (state.length === 0)
       state.needReadable = true;
     // call internal read method
-    this._read(state.highWaterMark);
+    stream._read(state.highWaterMark);
     state.sync = false;
   }

@@ -350,10 +355,10 @@ Readable.prototype.read = function(n) {

   // If we tried to read() past the EOF, then emit end on the next tick.
   if (nOrig !== n && state.ended && state.length === 0)
-    endReadable(this);
+    endReadable(stream);

   if (ret !== null)
-    this.emit('data', ret);
+    stream.emit('data', ret);

   return ret;
 };
@@ -731,7 +736,7 @@ function flow(stream) {
   debug('flow', state.flowing);
   if (state.flowing) {
     do {
-      var chunk = stream.read();
+      var chunk = stream.read(-1);
     } while (null !== chunk && state.flowing);
   }
 }

[Fishrock123]

As @mscdex suggested, this patch should do. I don't have access to linux perf tools, can someone check it?

diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js
index 1de5559..731973f 100644
--- a/lib/_stream_readable.js
+++ b/lib/_stream_readable.js
@@ -211,7 +211,8 @@ function howMuchToRead(n, state) {
   if (state.objectMode)
     return n === 0 ? 0 : 1;

-  if (n === null || isNaN(n)) {
+  // fast check for NaN with val !== val
+  if (n === null || n !== n) {
     // only flow one buffer at a time
     if (state.flowing && state.buffer.length)
       return state.buffer[0].length;

@chrisdickinson the typeof should inline quite well, I doubt there'd be any valuable perf gain from that patch.

[chrisdickinson]

@Fishrock123 The typeof should inline, but by calling the howMuchToRead function with both undefined and number types it (needlessly) becomes polymorphic, the elimination of which was the goal behind coercing n === undefined to n = -1 in Readable.prototype.read.

[Fishrock123]

but by calling the howMuchToRead function with both undefined and number types it (needlessly) becomes polymorphic

Ahh. I guess that'd be a separate thing to pref check then though.

[Trott]

Is this fixed/closed by #1925? Or is this still an issue?

[bnoordhuis]

It still comes up in profiles from time to time but it hasn't bothered me enough yet to revisit #1925. If you want to close it, you have my blessing.

[Trott]

OK, I'll close it then. It can always be re-opened. Thanks.