fs.readFile is slower in v10

[zbjornson]

• Version: v10.15.0
• Platform: Ubuntu 16, Win 10. Haven't tested macOS
• Subsystem: fs

I'm seeing a 7.6-13.5x drop in read throughput between 8.x and 10.x in both the readfile benchmark and our real-world benchmarks that heavily exercise fs.readFile. Based on my troubleshooting, I think it's from #17054.

The readfile benchmark (Ubuntu 16):

Test	v8.15.0	v10.15.0	8 ÷ 10
concurrent=1 len=1024	6,661	7,066	1.06x
concurrent=10 len=1024	23,100	21,079	0.91x
concurrent=1 len=16777216	156.6	11.6	13.5x
concurrent=10 len=16777216	584	76.6	7.6x

From what I can extract from the comments in #17054, either no degradation or a 3.6-4.8x degradation was expected for the len=16M cases.

As for why I think it's because of #17054, the benchmark below compares fs.readFile against an approximation of how fs.readFile used to work (one-shot read), measuring time to read the same 16 MB file 50 times.

// npm i async

const fs = require("fs");
const async = require("async");

function chunked(filename, cb) {
	fs.readFile(filename, cb);
}

function oneshot(filename, cb) {
	fs.open(filename, "r", 0o666, (err, fd) => {
		if (err) return cb(err);
		const onerr = err => fs.close(fd, () => cb(err));
		fs.fstat(fd, (err, stats) => {
			if (err) return onerr(err);
			const data = Buffer.allocUnsafe(stats.size);
			fs.read(fd, data, 0, stats.size, 0, (err, bytesRead) => {
				if (err) return onerr(err);
				if (bytesRead !== stats.size) {
					const err = new Error("Read fewer bytes than requested");
					return onerr(err);
				}
				fs.close(fd, err => cb(err, data));
			});
		});
	});
}

fs.writeFileSync("./test.dat", Buffer.alloc(16e6, 'x'));

function bm(method, name, cb) {
	const start = Date.now();
	async.timesSeries(50, (n, next) => {
		method("./test.dat", next);
	}, err => {
		if (err) return cb(err);
		const diff = Date.now() - start;
		console.log(name, diff);
		cb();
	});
}

async.series([
	cb => bm(chunked, "fs.readFile()", cb),
	cb => bm(oneshot, "oneshot", cb)
])

Node.js	OS	fs.readFile() (ms)	one-shot (ms)
v10.15.0	Ubuntu 16	7320	370
v8.15.0	Ubuntu 16	693	378
v10.15.0	Win 10	2972	493

We've switched to fs.fstat() and then fs.read() as a work-around (above; note how many lines of code it is), but I wouldn't be surprised if this has negatively impacted other apps/tools. As far as the original justification: I'm not convinced that was an appropriate fix or a problem that needed fixing. Web servers (where DoS attacks are relevant) should generally be using fs.createReadStream.pipe(response) for serving files (ignoring other use cases for now). Other sorts of apps like build tools, compilers and test frameworks (DoS attacks irrelevant) are the ones that more often need to read an entire file as fast as possible. In some senses, the fix made the DoS situation worse by increasing overhead (reducing number of useful CPU cycles) and by infinitely partitioning reads (each new request delays all existing requests -- the requests are waiting for their own ticks plus some number of ticks from all of the other requests). We built our web app to load-shed while maintaining quality of service for as many clients as possible, so interleaving is not what we want. -- Happy to discuss that more on- or off-line.

Is anyone else able to verify that this degradation exists and/or was expected?

cc: @davisjam

[addaleax]

I agree that #17054 seems likely as a source of the issue, and I agree with your reasoning re: stream usage.

[jorangreef]

@zbjornson I think your benchmark is artificial in that you're measuring the latency of a 16mb readFile() vs reading 16mb in one shot?

For the general case, readFile() should definitely not be reading 16mb in one shot, since this would affect not just the latency of a single readFile() call (which would improve), but also the throughput of the Node.js threadpool and system as a whole (which would degrade).

I think #17054 was a brilliant contribution.

We built our web app to load-shed while maintaining quality of service for as many clients as possible, so interleaving is not what we want.

In fact, your moving to fs.read() is the right way to go about this. You can express your buffering intention more clearly, since you are in a position to know the latency and throughput characteristics you are targeting much better than Node. Node can't know what kind of service you're running, or whether you want to read 16mb in one shot or not. Reading 16mb in one shot should not be the default.

Perhaps fs.readFile() could accept a partition option to set the size of the buffer used for reading, but I don't think #17054 should be blamed here.

[antsmartian]

Did a quick dirty check, I reverted the change #17054, so now the example benchmark looks like:

//node master
fs.readFile() 2358
oneshot 459

//after the revert - node master
fs.readFile() 475
oneshot 492

Here is the diff BTW:

diff --git a/lib/internal/fs/read_file_context.js b/lib/internal/fs/read_file_context.js
index d7543ffa5a..341d00c148 100644
--- a/lib/internal/fs/read_file_context.js
+++ b/lib/internal/fs/read_file_context.js
@@ -79,7 +79,7 @@ class ReadFileContext {
     } else {
       buffer = this.buffer;
       offset = this.pos;
-      length = Math.min(kReadFileBufferLength, this.size - this.pos);
+      length = this.size - this.pos;
     }
 
     const req = new FSReqCallback();

[mcollina]

I think

node/lib/internal/fs/read_file_context.js

Line 7 in 26f80dc

const kReadFileBufferLength = 8 * 1024;

might need to be increased for this use case, as 8KB seems not big enough to me to balance the two interests. Providing an option to set it might be a good call, as moving it to somewhere between 64KB to 512KB might provide better compromises.

[jdalton]

Does this affect fs.readFileSync too?

[zbjornson]

@jdalton no, fs.readFileSync benchmarks the same between v8.x and v10.x.

@jorangreef The benchmark reflects a real-world use case (asynchronously read a file as fast as possible in a scenario where this type of concurrency isn't an issue and/or it's an issue that can be mitigated by increasing the threadpool size). I'd love to know where fs.readFile() is used in web applications currently, and if any of those cases are ones that shouldn't be using fs.createReadStream() (which effectively partitions).

Node can't know what kind of service you're running, or whether you want to read 16mb in one shot or not.

This is equally valid to support not partitioning reads. Note in my OP that it's 16 LOC for (I think) a safe implementation of a non-partitioning read, while concatenating a stream is only 5 LOC (below). For that reason, it would be nicer for Node.js to provide a one-shot method and let users create a partitioning method.

function readPartitioned(filename, cb) {
  const rs = fs.createReadStream(filename);
  const buffs = [];
  fs.on("data", d => buffs.push(d));
  fs.on("error", e => cb(e));
  fs.on("end", () => cb(null, Buffer.concat(buffs)));
}

[affects] the throughput of the Node.js threadpool and system as a whole (which would degrade).

I did sort of a poor job trying to explain this in my OP, but partitioning still causes degradation, and in some senses, causes worse degradation due to the increased overhead.

[jorangreef]

@zbjornson ,

Your argument is kind of like saying TCP should not have congestion control. A single TCP connection should be given maximum bandwidth, to download a file "as fast as possible".

The problem with this, is that it ignores TCP as a system of connections, and allows a single connection to starve all others. I don't think that leaving congestion control out of TCP is a good default, most users of TCP won't realize their need for it, and the Internet might collapse.

Sure, congestion control has an effect on single connection download latency, but it's what most users of TCP want (whether they know it or not). If you really want "fast as possible" downloads, you are always free to use something custom such as UDT, and then I think metrics such as LOC are not the biggest concern.

readFile() should remain partitioned for the same reasons, and @mcollina has already provided the way forward.

[zbjornson]

(TCP congestion control is adaptive to network conditions and is designed to allow large segments when there is low congestion to maximize throughput. Always partitioning fs reads is like always running with a small segment size. You also generally can't quickly add more bandwidth to a network to mitigate congestion, whereas you can increase the threadpool size, number of cluster workers and/or number of servers running a web app.)

If increasing the partition size works, then that's a perfect solution. But my point is that the primary use cases for fs.readFile I think are not in web servers (rather in build tools, test frameworks, compilers), so partitioning unnecessarily deoptimizes those major uses cases.

[mcollina]

I disagree. Avoiding partitioning optimizes with the usecase of “only one read at a time”, and in fact you could potentially replace that with a single readFileSync call.
Asking for a single thread to block to read a 1GB file is problematic.

The problem we are facing atm is due to the high cost of moving control back-and-forth between JS and C++. Increasing the default chunk size and making it customizable will help significantly.

[zbjornson]

Offered for discussion: here's an alternative solution that uses aio_read(3). https://github.com/zbjornson/node-aio

That has nearly the same performance as a one-shot read, and unblocks the event loop almost entirely, while (I think) working around the issues that prevented libuv from using that method in the first place.

If folks think that's a viable solution, I'd be happy to work on a PR for it.

[Trott]

Offered for discussion: here's an alternative solution that uses aio_read(3). https://github.com/zbjornson/node-aio

That has nearly the same performance as a one-shot read, and unblocks the event loop almost entirely, while (I think) working around the issues that prevented libuv from using that method in the first place.

If folks think that's a viable solution, I'd be happy to work on a PR for it.

@nodejs/libuv

[bnoordhuis]

aio_read() uses threads too (or rather, libaio does) so no silver bullet there. :)