perf(runtime/fs): optimize readFile by using a single large buffer

[AaronO]

This avoids allocating N buffers when reading entire files and copying when concatenating them.

Benchmarks

# Before
❯ deno run -A ./cli/bench/deno_common.js
read_128k_sync:      	n = 50000, dt = 1.941s, r = 25760/s, t = 38820ns/op
read_128k:           	n = 50000, dt = 9.613s, r = 5201/s, t = 192259ns/op

# After
❯ ./target/release/deno run -A ./cli/bench/deno_common.js
read_128k_sync:      	n = 50000, dt = 1.746s, r = 28637/s, t = 34920ns/op
read_128k:           	n = 50000, dt = 8.865s, r = 5640/s, t = 177300ns/op

Notes

• The improvements to read_128k_sync are somewhat reduced here by the extra cwd lookups caused by the stat, so perf(runtime): cache cwd lookups #12056 will work hand in hand with this change to improve file reads
• This operates under the assumption that files won't change (truncated or extended) whilst being read, which isn't guaranteed but seems fair IMO
• Also simplifies implementation of readTextFile / readTextFileSync

[lucacasonato]

Not in favor of this. File reads area not atomic. We can't start optimizing at the cost of correctness.

[AaronO]

Not in favor of this. File reads area not atomic. We can't start optimizing at the cost of correctness.

My point is more that there's no sane use-case to write & read-all at the same time when you don't control the rate of reading and it's uncommon and IMO would be mostly fine as is.

But as I mentioned we can make this more robust:

Files being truncated should already be handled, files being extended could be handled, either by allocating an extra byte in the buffer and seeing if that is set or by doing an extra read.

essentially the current implementation would be a fast path for an unchanged file and then it would fallback to the slow path if the file was extended, so that wouldn't hurt or change correctness

[lucacasonato]

With fallback, SGTM

[piscisaureus]

FWIW, node.js does exactly this. https://github.com/nodejs/node/blob/bbd4c6eee90ef600022c72316ba82d37a2ae16bc/lib/fs.js#L326-L349

[AaronO]

I still think that it would be sane/fair to not read beyond the stat'd size, I can't imagine a sane use-case for it but I implemented the slowpath fallback anyway.

@bartlomieju @lucacasonato That should address your concerns

[lucacasonato]

Suggested change

	} else { // cursor == size
	} else { // cursor <= size

[lucacasonato]

LGTM