10 ms·

fs.promises.readFile is 40% slower than fs.readFile

145 points by Jarred 6y ago

beaconstudios 6y ago
const readFilePromise = (...args) => new Promise((resolve, reject) => readFile(...args, (err, result) => err ? reject(err) : resolve(result))) How can the native implementation be much more complex than that, beyond maybe some argument assertions? That will take almost the exact same amount of time as readFile, the additional time cost is a small constant value.
- orf 6y ago
  On a SSD reading files is fast. However read() is a blocking operation that could take forever - reading some /dev/ device that never returns any data, or a slower medium that takes hundreds of milliseconds per read. That’s a big no-no. So it gets dispatched to a dedicated blocking-io threadpool with limited workers which does the blocking operation without interrupting the event loop. I’m guessing as disk speeds have increased the relative overhead of dispatching the task to a thread pool has also increased. At least, that’s how I think it works based on some half-remembered blog post. Edit: yep - http://docs.libuv.org/en/v1.x/design.html#file-i-o http://docs.libuv.org/en/v1.x/design.html#file-i-o
  - beaconstudios 6y ago
    I can fully believe that there's some cleverness going on with node file I/O - but there should be no meaningful difference in performance between returning the result via a Promise, versus a callback.
    - orf 6y ago
      It depends - I would assume the callback pattern is much easier to JIT than the promise pattern and requires less objects to be allocated, but you’re right. That’s a huge difference. You’d need to benchmark another non-io bound function to find how much overhead comes from promises themselves.
      - beaconstudios 6y ago
        Not enough to add 40% overhead to a 1mb file read, I can tell you that much.
        javajosh 6y ago
        It depends. If disks got so fast that function call overhead starts to dominate, then you might see an effect like this. But I have to imagine reading from an SSD is still 1000x slower (at least) than a function invocation in Javascript.
        beaconstudios 6y ago
        I don't see that basically ever being the case.
    - SavantIdiot 6y ago
      The most recent comment is insightful: "My guess is that the promise version is idling when IO capacity is available due to scheduling? Is it implemented in an event driven way internally, and if it is (for example using epoll) is the epoll event able to interrupt and cause the promise to continue to be evaluated or does it have to wait for the vm to get around to scheduling it again?" It didn't occur to me that the async promise is rescheduled. That right there is a concern. The OP also points out buffer concatenation, which is an odd choice. Reading the file into memory in one go should start with an stat and then a single buffer alloc. But that is probably naive.
  - hinkley 6y ago
    Network filesystems, compressed filesystems, and of course the ever-dreaded on-access virus scanners on Windows that 'break' one open source project after another. Who knew that opening the same file 10 times is slow? Everybody. Everybody knew that. Looking at you, svn team.
  - TheCoelacanth 6y ago
    But these are both non-blocking APIs. readFileSync is the blocking version.
    - tynorf 6y ago
      I believe orf is talking about the read(2) syscall, which is blocking.
- Jarred 6y ago
  I agree in principle, but for some reason, the underlying implementation for fs.promises is completely separate. I think this is where fs.promises.readFile reads the actual file https://github.com/nodejs/node/blob/master/lib/internal/fs/promises.js#L319-L339 https://github.com/nodejs/node/blob/master/lib/internal/fs/p..., with many layers of indirection before that fs.readFile is this one function: https://github.com/nodejs/node/blob/master/lib/fs.js#L327-L360 https://github.com/nodejs/node/blob/master/lib/fs.js#L327-L3...
  - wcarss 6y ago
    I'm brand new to the code, but I don't think it is as simple as you state. The 'one function' you linked to just passes a ReadFileContext into binding.open[1]. The ReadFileContext appears to have its own read that uses its own allocated Buffers[2] under some circumstances, but under others they are mysteriously already there[3]. Then binding.open refers to an internalBinding of fs[4], which I'm unsure of, is that js or C++ code? This[5] suggests C++ is involved, but there is also an open in lib/fs.js[6] with that signature, though it also just calls binding.open at the end with the same signature, so I think it's off into C++ by then. At that point, I lose the trail. Maybe C++ is even doing the allocating. This is not easy code to grok in its full extent. 1 - https://github.com/nodejs/node/blob/master/lib/fs.js#L356 https://github.com/nodejs/node/blob/master/lib/fs.js#L356 2 - https://github.com/nodejs/node/blob/master/lib/internal/fs/read_file_context.js#L103 https://github.com/nodejs/node/blob/master/lib/internal/fs/r... 3 - https://github.com/nodejs/node/blob/master/lib/internal/fs/read_file_context.js#L108 https://github.com/nodejs/node/blob/master/lib/internal/fs/r... 4 - https://github.com/nodejs/node/blob/master/lib/fs.js#L74 https://github.com/nodejs/node/blob/master/lib/fs.js#L74 5 - https://github.com/nodejs/node/blob/3b2863db12818778671713b5c30363f788912aae/lib/internal/bootstrap/loaders.js#L21 https://github.com/nodejs/node/blob/3b2863db12818778671713b5... 6 - https://github.com/nodejs/node/blob/master/lib/fs.js#L472 https://github.com/nodejs/node/blob/master/lib/fs.js#L472 edit: I was curious so I kept reading, It seems like open is probably C++ here[7], which makes an AsyncCall or SyncCall to "open", which are just wrappers around syscalls[8]. Still not sure about other places the buffer comes from, though. 7 - https://github.com/nodejs/node/blob/master/src/node_file.cc#L1709 https://github.com/nodejs/node/blob/master/src/node_file.cc#... 8 - https://github.com/nodejs/node/blob/606df7c4e79324b9725bfcfe019a8b75bfa04c3f/src/node_file-inl.h#L267 https://github.com/nodejs/node/blob/606df7c4e79324b9725bfcfe...
- idbehold 6y ago
  I believe the difference here is that these newly introduced promise wrappers allow you to pass an AbortSignal instances to them. Wrapping the old callback style methods would not allow you to "abort" in the middle of reading a file. You could respond to a passed in AbortSignal instance in the simple `readFilePromise` function you provided by simply rejecting early, but it wouldn't free up the underlying file descriptor until after the entire file had been read.
  - idbehold 6y ago
    Actually I was completely wrong. They added support for consuming an AbortSignal in the old callback style methods as well. Now I'm at a loss as to why they even have two different implementations instead of just doing something like: fs.promises.readFile = util.promisify(fs.readFile);
xiphias2 6y ago
What really disturbs me is this comment: ,,But no one said fs.promises.readFile should perform similarly to fs.readFile '' I'm happy that it was downvoted at least, but there should be a better way to set expectations for excellence in development.
- arghwhat 6y ago
  I fear that the individual might even be serious...
  - ncallaway 6y ago
    It seemed pretty clear that it's a joke to me, with the thinky-face emoji after it, but that's just my reading of it.
    - arghwhat 6y ago
      Last I checked that wasn't an indicator for sarcasm, but maybe GitHub emoji culture has just gotten away from me.
      - bryanrasmussen 6y ago
        I don't know, I might say it as a joke, therefore seeing it said I assume they say it in the same manner as I would with the sound of innocent wonder that can be used to denote sarcasm. I think as a general rule it is good to assume the best, even though I myself might not always be able to do so because of various limitations of understanding.
- CyberRabbi 6y ago
  Well his point is this technically isn’t a bug because comparable performance to fs.readFile was never guaranteed as part of the API contract. I don’t think his statement implies we generally ought not to expect excellence nor that effort shouldn’t be spent on improving things. I think his point is valid even if slightly tongue in cheek. Bugs are defined by the contract / specification not wishful thinking.
spullara 6y ago
Edit: see comment below, misunderstood the specific case.
- bastawhiz 6y ago
  This doesn't compare async to sync: in fact, `readFileSync` is also benchmarked here, and is slower than every async alternative. This compares `readFile` (async) with `promises.readFile` (also async).
  - spullara 6y ago
    Even in that case, you are doing more work but 40% is excessive.
anonytrary 6y ago
const readFileP = (...a) => new Promise((res,rej)=>readFile(...a, (e,r)=>e?rej(e):res(r))); This is a one-line implementation and is probably way faster. I would totally just use this one-liner anyway.
- SavantIdiot 6y ago
  Did you benchmark it? OP provided the test code. Show us the data.
  - anonytrary 6y ago
    I did just because you asked, and mine (readFileP) is faster, barely different than the callback version. Actually it's the fastest one which makes no sense, so I think this benchmark may have quite a bit of error. Results: fs.readFileSync x 696 ops/sec ±1.96% (76 runs sampled) fs.readFile x 2,045 ops/sec ±6.43% (72 runs sampled) fs.promises.readFile x 435 ops/sec ±1.76% (78 runs sampled) util.promisify(fs.readFile) x 2,053 ops/sec ±6.58% (67 runs sampled) readFileP x 2,182 ops/sec ±4.00% (67 runs sampled) fs.promises.readFile CACHED readFile x 438 ops/sec ±1.56% (79 runs sampled) Fastest is readFileP,util.promisify(fs.readFile),fs.readFile Slowest is fs.promises.readFile,fs.promises.readFile CACHED readFile index.js: const Benchmark=require("benchmark"),fs=require("fs"),path=require("path"),chalk=require("chalk"),util=require("util"),promisifed=util.promisify(fs.readFile),bigFilepath=path.resolve(__dirname,"./big.file"),suite=new Benchmark.Suite("fs"),readFileP=(...e)=>new Promise((i,r)=>fs.readFile(...e,(e,l)=>{e?r(e):i(l)})),readFileC=fs.promises.readFile;suite.add("fs.readFileSync",e=>{fs.readFileSync(bigFilepath,"utf8"),e.resolve()},{defer:!0}).add("fs.readFile",e=>{fs.readFile(bigFilepath,(i,r)=>{e.resolve()})},{defer:!0}).add("fs.promises.readFile",e=>{fs.promises.readFile(bigFilepath).then(()=>{e.resolve()})},{defer:!0}).add("util.promisify(fs.readFile)",e=>{promisifed(bigFilepath).then(()=>{e.resolve()})},{defer:!0}).add("readFileP",e=>{readFileP(bigFilepath).then(()=>{e.resolve()})},{defer:!0}).add("fs.promises.readFile CACHED readFile",e=>{readFileC(bigFilepath).then(()=>{e.resolve()})},{defer:!0}).on("cycle",function(e){console.log(String(e.target))}).on("complete",function(){console.log("Fastest is "+chalk.green(this.filter("fastest").map("name"))),console.log("Slowest is "+chalk.red(this.filter("slowest").map("name")))}).run({defer:!0}); This little bit is my "implementation": readFileP=(...e)=>new Promise((i,r)=>fs.readFile(...e,(e,l)=>{e?r(e):i(l)})) Run: npm install --save benchmark chalk && node ./index.js
wruza 6y ago
Why doesn’t benchmark suite use promise/async and instead resorts to some defer.resolve()? It’s not even cps-styled next().
- anonytrary 6y ago
  That doesn't matter. It's a constant across all tests and would likely not introduce much noise.
EGreg 6y ago
Well, did they deliver on their promise? I wonder why they have set out to make it slower!
minitech 6y ago
Some side notes: - The readFileSync in this benchmark has UTF-8 decoding (produces a string instead of a Buffer). That’s why it’s slower; not sure why it’s included. - The first guess/suggestion, > I suspect the cause is right here: [https://github.com/nodejs/node/blob/3b2863db12818778671713b5c30363f788912aae/lib/internal/fs/promises.js#L319-L339 https://github.com/nodejs/node/blob/3b2863db12818778671713b5...] > Instead of creating a new Buffer for each chunk, it could allocate a single Buffer and write to that buffer. I don't think Buffer.concat or temporary arrays are necessary. is already what the code does. Multiple chunks and Buffer.concat are only involved if more data gets read than stat initially reports.
- KoenDG 6y ago
  >The readFileSync in this benchmark has UTF-8 decoding Do you have benchmark results to show this?
jfrunyon 6y ago
Welcome to JavaScript.
timdaub 6y ago
Why is this on the front page? What's the message here?

145 points by Jarred 6y ago

const readFilePromise = (...args) => new Promise((resolve, reject) => readFile(...args, (err, result) => err ? reject(err) : resolve(result))) How can the native implementation be much more complex than that, beyond maybe some argument assertions? That will take almost the exact same amount of time as readFile, the additional time cost is a small constant value.

orf 6y ago
On a SSD reading files is fast. However read() is a blocking operation that could take forever - reading some /dev/ device that never returns any data, or a slower medium that takes hundreds of milliseconds per read. That’s a big no-no. So it gets dispatched to a dedicated blocking-io threadpool with limited workers which does the blocking operation without interrupting the event loop. I’m guessing as disk speeds have increased the relative overhead of dispatching the task to a thread pool has also increased. At least, that’s how I think it works based on some half-remembered blog post. Edit: yep - http://docs.libuv.org/en/v1.x/design.html#file-i-o http://docs.libuv.org/en/v1.x/design.html#file-i-o
- beaconstudios 6y ago
  I can fully believe that there's some cleverness going on with node file I/O - but there should be no meaningful difference in performance between returning the result via a Promise, versus a callback.
  - orf 6y ago
    It depends - I would assume the callback pattern is much easier to JIT than the promise pattern and requires less objects to be allocated, but you’re right. That’s a huge difference. You’d need to benchmark another non-io bound function to find how much overhead comes from promises themselves.
    - beaconstudios 6y ago
      Not enough to add 40% overhead to a 1mb file read, I can tell you that much.
      - javajosh 6y ago
        It depends. If disks got so fast that function call overhead starts to dominate, then you might see an effect like this. But I have to imagine reading from an SSD is still 1000x slower (at least) than a function invocation in Javascript.
        beaconstudios 6y ago
        I don't see that basically ever being the case.
  - SavantIdiot 6y ago
    The most recent comment is insightful: "My guess is that the promise version is idling when IO capacity is available due to scheduling? Is it implemented in an event driven way internally, and if it is (for example using epoll) is the epoll event able to interrupt and cause the promise to continue to be evaluated or does it have to wait for the vm to get around to scheduling it again?" It didn't occur to me that the async promise is rescheduled. That right there is a concern. The OP also points out buffer concatenation, which is an odd choice. Reading the file into memory in one go should start with an stat and then a single buffer alloc. But that is probably naive.
- hinkley 6y ago
  Network filesystems, compressed filesystems, and of course the ever-dreaded on-access virus scanners on Windows that 'break' one open source project after another. Who knew that opening the same file 10 times is slow? Everybody. Everybody knew that. Looking at you, svn team.
- TheCoelacanth 6y ago
  But these are both non-blocking APIs. readFileSync is the blocking version.
  - tynorf 6y ago
    I believe orf is talking about the read(2) syscall, which is blocking.
Jarred 6y ago
I agree in principle, but for some reason, the underlying implementation for fs.promises is completely separate. I think this is where fs.promises.readFile reads the actual file https://github.com/nodejs/node/blob/master/lib/internal/fs/promises.js#L319-L339 https://github.com/nodejs/node/blob/master/lib/internal/fs/p..., with many layers of indirection before that fs.readFile is this one function: https://github.com/nodejs/node/blob/master/lib/fs.js#L327-L360 https://github.com/nodejs/node/blob/master/lib/fs.js#L327-L3...
- wcarss 6y ago
  I'm brand new to the code, but I don't think it is as simple as you state. The 'one function' you linked to just passes a ReadFileContext into binding.open[1]. The ReadFileContext appears to have its own read that uses its own allocated Buffers[2] under some circumstances, but under others they are mysteriously already there[3]. Then binding.open refers to an internalBinding of fs[4], which I'm unsure of, is that js or C++ code? This[5] suggests C++ is involved, but there is also an open in lib/fs.js[6] with that signature, though it also just calls binding.open at the end with the same signature, so I think it's off into C++ by then. At that point, I lose the trail. Maybe C++ is even doing the allocating. This is not easy code to grok in its full extent. 1 - https://github.com/nodejs/node/blob/master/lib/fs.js#L356 https://github.com/nodejs/node/blob/master/lib/fs.js#L356 2 - https://github.com/nodejs/node/blob/master/lib/internal/fs/read_file_context.js#L103 https://github.com/nodejs/node/blob/master/lib/internal/fs/r... 3 - https://github.com/nodejs/node/blob/master/lib/internal/fs/read_file_context.js#L108 https://github.com/nodejs/node/blob/master/lib/internal/fs/r... 4 - https://github.com/nodejs/node/blob/master/lib/fs.js#L74 https://github.com/nodejs/node/blob/master/lib/fs.js#L74 5 - https://github.com/nodejs/node/blob/3b2863db12818778671713b5c30363f788912aae/lib/internal/bootstrap/loaders.js#L21 https://github.com/nodejs/node/blob/3b2863db12818778671713b5... 6 - https://github.com/nodejs/node/blob/master/lib/fs.js#L472 https://github.com/nodejs/node/blob/master/lib/fs.js#L472 edit: I was curious so I kept reading, It seems like open is probably C++ here[7], which makes an AsyncCall or SyncCall to "open", which are just wrappers around syscalls[8]. Still not sure about other places the buffer comes from, though. 7 - https://github.com/nodejs/node/blob/master/src/node_file.cc#L1709 https://github.com/nodejs/node/blob/master/src/node_file.cc#... 8 - https://github.com/nodejs/node/blob/606df7c4e79324b9725bfcfe019a8b75bfa04c3f/src/node_file-inl.h#L267 https://github.com/nodejs/node/blob/606df7c4e79324b9725bfcfe...
idbehold 6y ago
I believe the difference here is that these newly introduced promise wrappers allow you to pass an AbortSignal instances to them. Wrapping the old callback style methods would not allow you to "abort" in the middle of reading a file. You could respond to a passed in AbortSignal instance in the simple `readFilePromise` function you provided by simply rejecting early, but it wouldn't free up the underlying file descriptor until after the entire file had been read.
- idbehold 6y ago
  Actually I was completely wrong. They added support for consuming an AbortSignal in the old callback style methods as well. Now I'm at a loss as to why they even have two different implementations instead of just doing something like: fs.promises.readFile = util.promisify(fs.readFile);

xiphias2 6y ago

What really disturbs me is this comment: ,,But no one said fs.promises.readFile should perform similarly to fs.readFile '' I'm happy that it was downvoted at least, but there should be a better way to set expectations for excellence in development.

arghwhat 6y ago
I fear that the individual might even be serious...
- ncallaway 6y ago
  It seemed pretty clear that it's a joke to me, with the thinky-face emoji after it, but that's just my reading of it.
  - arghwhat 6y ago
    Last I checked that wasn't an indicator for sarcasm, but maybe GitHub emoji culture has just gotten away from me.
    - bryanrasmussen 6y ago
      I don't know, I might say it as a joke, therefore seeing it said I assume they say it in the same manner as I would with the sound of innocent wonder that can be used to denote sarcasm. I think as a general rule it is good to assume the best, even though I myself might not always be able to do so because of various limitations of understanding.
CyberRabbi 6y ago
Well his point is this technically isn’t a bug because comparable performance to fs.readFile was never guaranteed as part of the API contract. I don’t think his statement implies we generally ought not to expect excellence nor that effort shouldn’t be spent on improving things. I think his point is valid even if slightly tongue in cheek. Bugs are defined by the contract / specification not wishful thinking.

spullara 6y ago

Edit: see comment below, misunderstood the specific case.

bastawhiz 6y ago
This doesn't compare async to sync: in fact, `readFileSync` is also benchmarked here, and is slower than every async alternative. This compares `readFile` (async) with `promises.readFile` (also async).
- spullara 6y ago
  Even in that case, you are doing more work but 40% is excessive.

anonytrary 6y ago

const readFileP = (...a) => new Promise((res,rej)=>readFile(...a, (e,r)=>e?rej(e):res(r))); This is a one-line implementation and is probably way faster. I would totally just use this one-liner anyway.

SavantIdiot 6y ago
Did you benchmark it? OP provided the test code. Show us the data.
- anonytrary 6y ago
  I did just because you asked, and mine (readFileP) is faster, barely different than the callback version. Actually it's the fastest one which makes no sense, so I think this benchmark may have quite a bit of error. Results: fs.readFileSync x 696 ops/sec ±1.96% (76 runs sampled) fs.readFile x 2,045 ops/sec ±6.43% (72 runs sampled) fs.promises.readFile x 435 ops/sec ±1.76% (78 runs sampled) util.promisify(fs.readFile) x 2,053 ops/sec ±6.58% (67 runs sampled) readFileP x 2,182 ops/sec ±4.00% (67 runs sampled) fs.promises.readFile CACHED readFile x 438 ops/sec ±1.56% (79 runs sampled) Fastest is readFileP,util.promisify(fs.readFile),fs.readFile Slowest is fs.promises.readFile,fs.promises.readFile CACHED readFile index.js: const Benchmark=require("benchmark"),fs=require("fs"),path=require("path"),chalk=require("chalk"),util=require("util"),promisifed=util.promisify(fs.readFile),bigFilepath=path.resolve(__dirname,"./big.file"),suite=new Benchmark.Suite("fs"),readFileP=(...e)=>new Promise((i,r)=>fs.readFile(...e,(e,l)=>{e?r(e):i(l)})),readFileC=fs.promises.readFile;suite.add("fs.readFileSync",e=>{fs.readFileSync(bigFilepath,"utf8"),e.resolve()},{defer:!0}).add("fs.readFile",e=>{fs.readFile(bigFilepath,(i,r)=>{e.resolve()})},{defer:!0}).add("fs.promises.readFile",e=>{fs.promises.readFile(bigFilepath).then(()=>{e.resolve()})},{defer:!0}).add("util.promisify(fs.readFile)",e=>{promisifed(bigFilepath).then(()=>{e.resolve()})},{defer:!0}).add("readFileP",e=>{readFileP(bigFilepath).then(()=>{e.resolve()})},{defer:!0}).add("fs.promises.readFile CACHED readFile",e=>{readFileC(bigFilepath).then(()=>{e.resolve()})},{defer:!0}).on("cycle",function(e){console.log(String(e.target))}).on("complete",function(){console.log("Fastest is "+chalk.green(this.filter("fastest").map("name"))),console.log("Slowest is "+chalk.red(this.filter("slowest").map("name")))}).run({defer:!0}); This little bit is my "implementation": readFileP=(...e)=>new Promise((i,r)=>fs.readFile(...e,(e,l)=>{e?r(e):i(l)})) Run: npm install --save benchmark chalk && node ./index.js

wruza 6y ago

Why doesn’t benchmark suite use promise/async and instead resorts to some defer.resolve()? It’s not even cps-styled next().

anonytrary 6y ago
That doesn't matter. It's a constant across all tests and would likely not introduce much noise.

EGreg 6y ago

Well, did they deliver on their promise? I wonder why they have set out to make it slower!

minitech 6y ago

Some side notes: - The readFileSync in this benchmark has UTF-8 decoding (produces a string instead of a Buffer). That’s why it’s slower; not sure why it’s included. - The first guess/suggestion, > I suspect the cause is right here: [https://github.com/nodejs/node/blob/3b2863db12818778671713b5c30363f788912aae/lib/internal/fs/promises.js#L319-L339 https://github.com/nodejs/node/blob/3b2863db12818778671713b5...] > Instead of creating a new Buffer for each chunk, it could allocate a single Buffer and write to that buffer. I don't think Buffer.concat or temporary arrays are necessary. is already what the code does. Multiple chunks and Buffer.concat are only involved if more data gets read than stat initially reports.

KoenDG 6y ago
>The readFileSync in this benchmark has UTF-8 decoding Do you have benchmark results to show this?

jfrunyon 6y ago

Welcome to JavaScript.

timdaub 6y ago

Why is this on the front page? What's the message here?