fs: improve readFile performance

[vkurchatkin]

This PR improves readFile performance by reducing number of closure allocations and using
FSReqWrap directly.

see #704

R=@trevnorris, @petkaantonov

[trevnorris]

Mostly LGTM. I still need to test it. Are we going to slowly move everything over to let?

[micnic]

I think we could use var in functions context and global context, but let inside if / for / while statements. Like this we will know for sure what kind of variables are being used, block-scoped or not.

[timoxley]

@micnic note the messaging seems to me that let & const should be used wherever possible; var is basically deprecated or at least not advised. If you want to initialise something inside an if, just promote the let/constdeclaration to the outer closure.

[timoxley]

Are we going to slowly move everything over to let?

@trevnorris note that at current, let appears significantly slower than var:

// in io.js 1.1.0 repl
(function() {
  (function() {
    "use strict"
    console.time('let')
    let result = 0
    for (let i = 0; i < 10000000; i++) {
      let lettest = 'foo bar baz';
      let x = i * i
      result += x
    }
    console.timeEnd('let')
    return result
  })();

  (function() {
    "use strict"
    console.time('var')
    var result = 0
    for (var i = 0; i < 10000000; i++) {
      var lettest = 'foo bar baz'
      var x = i * i
      result += x
    }
    console.timeEnd('var')
    return result
  })()
})()
// let: 1183ms
// var: 15ms

Maybe this benchmark is bogus though.

[micnic]

@timoxley , var isn't deprecated or not advised, yes, let can replace var everywhere, but I meant that we can use together var and let for some semantic reasons.

[vkurchatkin]

It seems that the only reasons to introduce let is backwards compatibility. Block scoping is something that is wanted in most of situations. Popular example:

for (var i = 0; i < 10; i++) {
  setImmediate(function() {
    console.log(i);
  })
}

Fixed simply by swapping var for let. Using var inside of blocks is generally confusing in the same way function declarations are (and they are prohibited in strict mode).

That said, if let is actually slower than var it's probably to early to use it.

[bnoordhuis]

@timoxley I wouldn't say it's bogus. You can see with --trace_gc --trace_osr where the difference comes from: manipulating the result let binding from an inner scope seems to prevent on-stack replacement (OSR), stopping the function from getting optimized. That in turn means the heap allocation for i * i isn't optimized away, leading to a ton of garbage collector activity.

I believe that's mostly been fixed in V8 HEAD but for now, we should probably scale back a little on const/let-ification, at least until we upgrade again.

[vkurchatkin]

Removed all lets

[bnoordhuis]

It would be even neater to fold the FSReqWrap object into the ReadFileContext object. Something for another time, though.

[trevnorris]

All the FSReqWrap() object are one use, which means they could be reused for each step. Preventing the need to instantiate more objects. Food for thought.

[bnoordhuis]

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/152/

[trevnorris]

I'd recommend changing this to new SlowBuffer(8192);. Just do a var SlowBuffer = require('buffer').SlowBuffer; at the top of the file.

[vkurchatkin]

I'm not sure that reusing FSReqWrap is safe, but I've tried and everything seems to work fine. Also switched to SlowBuffer and a couple of minor tweaks. PTAL

[trevnorris]

@vkurchatkin Awesome job on the callback hoisting BTW. That had been bothering me for a while.

Changes LGTM.

[vkurchatkin]

@trevnorris can you confirm that using one FSReqWrap for everything is ok? a bit worried about this

[trevnorris]

@vkurchatkin ah. no, definitely not safe doing that right now. will require some additional code. let's save that for another PR.

[vkurchatkin]

@trevnorris thank! dropped that commit, fixed some style issues, squashed and landed in e653080

[trevnorris]

@vkurchatkin great! thanks for your work on this.

[rvagg]

do we have any perf numbers on this for a changelog entry @vkurchatkin? otherwise it's just handwavy.

[trevnorris]

@rvagg Numbers would be nice, but if the performance numbers are the same having hoisted out the callbacks is a big win for doing performance analysis.

[vkurchatkin]

it's sort of hard to measure. It doesn't make reading files significantly faster, but it's supposed to block main thread less

[bnoordhuis]

I'd recommend changing this to new SlowBuffer(8192);. Just do a var SlowBuffer = require('buffer').SlowBuffer; at the top of the file.

@trevnorris I was looking at lib/fs.js and lib/buffer.js today and I confess it's not immediately obvious to me why a SlowBuffer is better than a regular Buffer.

Secondly, I think I spotted a minor bug. This PR calls new SlowBuffer(size). Here is the implementation of SlowBuffer:

function SlowBuffer(length) {
  length = length >>> 0;
  if (length > kMaxLength) {
    throw new RangeError('Attempt to allocate Buffer larger than maximum ' +
                         'size: 0x' + kMaxLength.toString(16) + ' bytes');
  }
  var b = new NativeBuffer(length);
  alloc(b, length);
  return b;
}

I think the new keyword is superfluous?

[vkurchatkin]

@bnoordhuis It's better because kReadFileBufferLength === Buffer.poolSize by default, I guess

[trevnorris]

@bnoordhuis hah, yeah. you're right about SlowBuffer(). Guess the new can be omitted. And the use of SlowBuffer is as @vkurchatkin said. Since kReadFileBufferLength > Buffer.poolSize / 2 by default it's faster to just bypass the type checks at the beginning of Buffer() and allocate memory. Not much, but it's there.