Skip to content
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

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

Open
Jarred-Sumner opened this issue Mar 3, 2021 · 10 comments
Open

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

Jarred-Sumner opened this issue Mar 3, 2021 · 10 comments

Comments

@Jarred-Sumner
Copy link

@Jarred-Sumner Jarred-Sumner commented Mar 3, 2021

  • Version: 15.10.0
  • Platform: macOS
  • Subsystem: 11.2.1

What steps will reproduce the bug?

Run this benchmark on a 1 MB file (big.file):

const Benchmark = require("benchmark");
const fs = require("fs");
const path = require("path");
const chalk = require("chalk");
const util = require("util");
const promisifed = util.promisify(fs.readFile);

const bigFilepath = path.resolve(__dirname, "./big.file");

const suite = new Benchmark.Suite("fs");

suite
  .add(
    "fs.readFileSync",
    (defer) => {
      fs.readFileSync(bigFilepath, "utf8");
      defer.resolve();
    },
    { defer: true }
  )
  .add(
    "fs.readFile",
    (defer) => {
      fs.readFile(bigFilepath, (err, data) => {
        defer.resolve();
      });
    },
    { defer: true }
  )
  .add(
    "fs.promises.readFile",
    (defer) => {
      fs.promises.readFile(bigFilepath).then(() => {
        defer.resolve();
      });
    },
    { defer: true }
  )
  .add(
    "util.promisify(fs.readFile)",
    (defer) => {
      promisifed(bigFilepath).then(() => {
        defer.resolve();
      });
    },
    { defer: true }
  )
  .on("cycle", function (event) {
    console.log(String(event.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: true });

To create a 1 MB file (~40% slower):

dd if=/dev/zero of=big.file count=1024 bs=1024

To create a 20 KB file (~55% slower):

dd if=/dev/zero of=big.file count=20 bs=1024

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

fs.promises.readFile should perform similarly to fs.readFile

What do you see instead?

image

Additional information

I suspect the cause is right here: https://github.com/nodejs/node/blob/master/lib/internal/fs/promises.js#L319-L339

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.

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Mar 3, 2021

Would you be interested in sending a PR to improve the implementation?

@TheRamann
Copy link

@TheRamann TheRamann commented Mar 3, 2021

But no one said fs.promises.readFile should perform similarly to fs.readFile 🤔

@Sylack-lab
Copy link

@Sylack-lab Sylack-lab commented Mar 3, 2021

Well that's right, but no one said it had to be slower either. I think all code should always be as fast as possible.

@justinvanwinkle
Copy link

@justinvanwinkle justinvanwinkle commented Mar 3, 2021

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?

@vigneshtdev
Copy link

@vigneshtdev vigneshtdev commented Mar 4, 2021

What about other resources(CPU/memory) consumption?

@Sylack-lab
Copy link

@Sylack-lab Sylack-lab commented Mar 4, 2021

What about other resources(CPU/memory) consumption?

I dont think it's that, but cant be discarded.

The best option here it's to reproduce.

@Linkgoron
Copy link
Member

@Linkgoron Linkgoron commented Mar 4, 2021

Note that it looks like there are some other differences as well, although I'm not sure how much they affect performance:

  • It looks like the fs/promises version does an extra read even if it has already read size amount of bytes (it doesn't keep track of how much it has read in total), and only finishes reading when it reads 0 bytes. The fs version checks if it has read size amount and ends the read (readFileAfterRead in read_file_context.js)
  • chunk sizes are a bit different (2**14 vs 512*1024)
  • Buffer.alloc vs Buffer.allocUnsafeSlow

Also, the 'utf8' in your readFileSync is really slowing it down, on my machine if you remove it the sync version is the fastest by far (and the utils.promisify(fs.readFile) is usually faster than fs.readFile).

@medikoo
Copy link

@medikoo medikoo commented Mar 5, 2021

(and the utils.promisify(fs.readFile) is usually faster than fs.readFile).

This signals the benchmark flaw. as it's same to stating that decorate(fn)() is faster than fn() (ofc assuming that in both cases fn() does exactly same job).

Promises naturally will always be slower than callbacks (extra objects to garbage collect and artificial ticks are not free)

on my machine if you remove it the sync version is the fastest

I can expect it when running 1 to 1, but I'd expect callback version to be more efficient (faster) if we start to process multiple files and take advantage of parallel execution. If that's not the case, I'd see it as other flaw on Node.js side

@thelebdev
Copy link

@thelebdev thelebdev commented Mar 5, 2021

This is an amazing finding, @Jarred-Sumner
I have a quick question, what did you use to benchmark it and come up with the tests?

Linkgoron added a commit to Linkgoron/node that referenced this issue Mar 5, 2021
Improve the fsPromises readFile performance
by allocating only one buffer, when size is known,
increase the size of the readbuffer chunks,
and dont read more data if size bytes have been read

refs: nodejs#37583
@Sylack-lab
Copy link

@Sylack-lab Sylack-lab commented Mar 8, 2021

This is an amazing finding, @Jarred-Sumner
I have a quick question, what did you use to benchmark it and come up with the tests?

Quite sure it's npm package "benchmark", sorry if I did not understand your question and you meant something else

benjamingr added a commit that referenced this issue Mar 10, 2021
Improve the fsPromises readFile performance
by allocating only one buffer, when size is known,
increase the size of the readbuffer chunks,
and dont read more data if size bytes have been read

Refs: #37583
PR-URL: #37608
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins added a commit that referenced this issue Mar 10, 2021
Improve the fsPromises readFile performance
by allocating only one buffer, when size is known,
increase the size of the readbuffer chunks,
and dont read more data if size bytes have been read

Refs: #37583
PR-URL: #37608
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Linkgoron added a commit to Linkgoron/node that referenced this issue Mar 10, 2021
Improve the fsPromises readFile performance
by allocating only one buffer, when size is known,
increase the size of the readbuffer chunks,
and dont read more data if size bytes have been read

refs: nodejs#37583
Linkgoron added a commit to Linkgoron/node that referenced this issue Mar 10, 2021
Improve the fsPromises readFile performance
by allocating only one buffer, when size is known,
increase the size of the readbuffer chunks,
and dont read more data if size bytes have been read

refs: nodejs#37583
Backport-PR-URL: nodejs#37608
Linkgoron added a commit to Linkgoron/node that referenced this issue Mar 11, 2021
Improve the fsPromises readFile performance
by allocating only one buffer, when size is known,
increase the size of the readbuffer chunks,
and dont read more data if size bytes have been read

refs: nodejs#37583
Backport-PR-URL: nodejs#37703
PR-URL: nodejs#37608
Linkgoron added a commit to Linkgoron/node that referenced this issue Apr 9, 2021
Improve the fsPromises readFile performance
by allocating only one buffer, when size is known,
increase the size of the readbuffer chunks,
and dont read more data if size bytes have been read

refs: nodejs#37583
Backport-PR-URL: nodejs#37703
PR-URL: nodejs#37608
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants