fs: introduce `filehandle.stream` #38440

Ayase-252 · 2021-04-27T16:16:44Z

This PR introduces a new API stream on filehandle, which has better performance over stream.pipe on file operation.

Benchmark

I conducted two benchmarking experiments about copying files by two different way, stream.pipe and filehandle.stream. For benchmarking code see benchmark/fs/bench-stream-via-pipeline.js and benchmark/fs/bench-stream.js. The result is showed as following.

File Size	Copy by`stream.pipe` (Q1)	Copy by `filehandle.stream` (Q2)	Improvement (Q2/Q1)
1 KB	2,882.30	11,102.54	3.85
1 MB	786.42	1,656.13	2.11
10 MB	68.30	73.16	1.07

Raw Result

➜  node git:(feature/add-stream-to-file-handle) out/Release/node benchmark/fs/bench-stream-via-pipeline.js
fs/bench-stream-via-pipeline.js fileSize=1024 n=100: 2,882.297241630011
fs/bench-stream-via-pipeline.js fileSize=1048576 n=100: 786.4203946112839
fs/bench-stream-via-pipeline.js fileSize=10485760 n=100: 68.301593019569
➜  node git:(feature/add-stream-to-file-handle) out/Release/node benchmark/fs/bench-stream.js
fs/bench-stream.js fileSize=1024 n=100: 11,102.539950269502
fs/bench-stream.js fileSize=1048576 n=100: 1,656.1340954553295
fs/bench-stream.js fileSize=10485760 n=100: 73.16234942556548

It shows good performance over small-size file (~1KB), and comparable performance over large-size file (~10MB).

cc @ronag

Fixes: #38350

Fixes: nodejs#38350

ronag · 2021-04-27T16:27:23Z

doc/api/fs.md

+  * `start` {integer} Optional, the position where stream starts from.
+    **Default:** `0`
+  * `end` {integer} Optional, the position where stream ends.
+    If it isn't provided. Stream will terminate at the EOF.


Is end inclusive or exclusive? Should it match with createReadStream?

ronag · 2021-04-27T16:28:13Z

lib/internal/fs/promises.js

+    }
+
+    const { buffer, bytesRead } = await handle.read(
+      allocatedBuffer,


You need to allocate a new buffer every time.

ronag · 2021-04-27T16:29:08Z

lib/internal/fs/promises.js

+    if (bytesRead === 0) {
+      break;
+    }
+


Also you need something like:

if (bytesRead !== buffer.length) { // Slow path. Shrink to fit. // Copy instead of slice so that we don't retain // large backing buffer for small reads. const dst = Buffer.allocUnsafeSlow(bytesRead) buffer.copy(dst, 0, 0, bytesRead) buffer = dst }

wa-Nadoo · 2021-04-27T18:09:59Z

Why is pipe()/pipeline() so slow?

aduh95

A few non-blocking comments to improve the benchmark setup and cleanup efficiency.

benchmark/fs/bench-stream-via-pipeline.js

benchmark/fs/bench-stream.js

benchmark/fs/bench-stream-via-pipeline.js

aduh95 · 2021-04-27T18:21:30Z

benchmark/fs/bench-stream-via-pipeline.js

+  async function cleanUp(i) {
+    await Promise.all([fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)]);


Suggested change

async function cleanUp(i) {

await Promise.all([fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)]);

function cleanUp(_, i) {

return [fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)];

aduh95 · 2021-04-27T18:22:27Z

benchmark/fs/bench-stream.js

+  async function cleanUp(i) {
+    await Promise.all([fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)]);


Suggested change

async function cleanUp(i) {

await Promise.all([fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)]);

function cleanUp(_, i) {

return [fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)];

lib/internal/fs/promises.js

benchmark/fs/bench-stream-via-pipeline.js

jasnell · 2021-04-27T19:23:54Z

doc/api/fs.md

@@ -368,6 +368,24 @@ changes:
    {fs.Stats} object should be `bigint`. **Default:** `false`.
 * Returns: {Promise} Fulfills with an {fs.Stats} for the file.

+#### `filehandle.stream(dst, [options])`


To be honest, I'd be happier with an API that would work with the existing streams API and pipeline utility. Specifically, something like:

pipeline(filehandle.readable(), dst, () => {}); pipeline(src, filehandle.writable(), () => {});

Where the return value of filehandle.readable() is essentially an fs.ReadStream and filehandle.writable() is an fs.WriteStream.

If we did want an API like this, the I would more expect filehandle.pipe(dst, [options]) or filehandle.pipeTo(dst, [options]).

Specifically, something like:

That kind of removes the point of this though. Will have the same performance as just doing fs.createReadStream.

Performance is not the only concern, however... a consistent and meaningful API that is not confusing is as equally important. What I'd rather see is effort into making fs.ReadStream and fs.WriteStream faster and for there to be a shared code path here so that overall maintainability, consistency, and performance are all improved.

jasnell · 2021-04-27T19:25:00Z

lib/internal/fs/promises.js

+  if (start && typeof start !== 'number') {
+    throw new ERR_INVALID_ARG_TYPE('start', 'number', start);
+  }
+  if (end && typeof end !== 'number') {
+    throw new ERR_INVALID_ARG_TYPE('end', 'number', end);
+  }
+  if (start < 0) {
+    throw new ERR_OUT_OF_RANGE('start', 'greater than 0', start);
+  }
+  if (end < 0) {
+    throw new ERR_OUT_OF_RANGE('end', 'greater than 0', end);
+  }
+  if (end && start > end) {
+    throw new ERR_OUT_OF_RANGE('start', `less than end ${end}`, start);
+  }


Should use existing validators.js here

jasnell · 2021-04-27T19:25:40Z

lib/internal/fs/promises.js

+  const {
+    start = 0,
+    end,
+  } = options;


this, and the checkAborted() above will fail if options is null or undefined

@jasnell The object options is specified as {} and validated via validateObject function in case of given invalid, so it cannot be null or undefined. Am I missing something?

lib/internal/fs/promises.js

addaleax · 2021-04-27T21:48:58Z

lib/internal/fs/promises.js

+  }
+  const BUFFER_SIZE = 128 * 2 ** 10;
+  const {
+    start = 0,


This is not a good default value, we should start reading from the current position instead of the start of the file by default.

addaleax · 2021-04-27T21:49:55Z

lib/internal/fs/promises.js

@@ -739,6 +747,68 @@ async function readFile(path, options) {
  return PromisePrototypeFinally(readFileHandle(fd, options), fd.close);
 }

+async function stream(handle, dest, options) {


If the function does the equivalent of .pipe(), then naming it stream is going to be confusing.

Ayase-252 · 2021-04-28T01:56:12Z

I would address reviews some time on the evening. 😄

Ayase-252 · 2021-04-28T13:57:14Z

FWIW, Current benchmarking result on f7f6039

File Size	Copy by `stream.pipe` (Q1)	Copy by `filehandle.stream` (Q2)	Improvement (Q2/Q1)
1 KB	4,495.32	17,491.61	3.89
1 MB	587.06	1,141.86	1.94
10 MB	69.03	87.62	1.27

The idea behind #38350 is simple. And it did give good benchmarking result (at least in my case). I think it is the value of the PR.

It could be implemented in user land easily for users whose application is performance-critical. I'm also fine to close this in favor of a more preferable way which integrates with current Node.js architecture better if this didn't so.

aduh95 · 2021-04-28T14:07:26Z

benchmark/fs/bench-stream-via-pipeline.js

+  }
+
+  async function cleanUp(_, i) {
+    await Promise.all([fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)]);


If you return the array instead of waiting for it, you avoid the creation of conf.n promises – instead the promises will be awaited on line 48.

Suggested change

await Promise.all([fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)]);

return [fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)];

aduh95 · 2021-04-28T14:08:08Z

benchmark/fs/bench-stream.js

+  }
+
+  async function cleanUp(_, i) {
+    await Promise.all([fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)]);


If you return the array instead of waiting for it, you avoid the creation of conf.n unnecessary promises – instead the promises will be awaited on line 45.

Suggested change

await Promise.all([fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)]);

return [fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)];

Thanks, it makes sense.

jasnell · 2021-04-28T14:25:35Z

lib/internal/fs/promises.js

+async function stream(handle, dest, options) {
+  if (!options) {
+    options = {};
+  }


I would make this just...

async function stream(handle, dest, options = {}) { validateObject(options, 'options'); // ...

Got it, thanks!

jasnell · 2021-04-28T14:26:16Z

lib/internal/fs/promises.js

+    end,
+  } = options;
+
+  if (start) {


Suggested change

if (start) {

if (start !== undefined) {

jasnell · 2021-04-28T14:26:25Z

lib/internal/fs/promises.js

+  if (start) {
+    validateInteger(start, 'start', 0);
+  }
+  if (end) {


Suggested change

if (end) {

if (end !== undefined) {

Co-authored-by: Antoine du Hamel <[email protected]>

Co-authored-by: James M Snell <[email protected]>

ronag · 2021-04-29T06:19:29Z

lib/internal/fs/promises.js

+      buffer = copy;
+    }
+
+    dest.write(buffer);


Suggested change

dest.write(buffer);

if (!dest.write(buffer)) {

if (dest.destroyed) return;

await EE.once(dest, 'drain');

}

Better compat with old streams.

ronag · 2021-04-29T06:19:54Z

lib/internal/fs/promises.js

+    if (dest.writableNeedDrain) {
+      await EE.once(dest, 'drain');
+    }


Suggested change

if (dest.writableNeedDrain) {

await EE.once(dest, 'drain');

}

if (dest.writableNeedDrain) {

if (dest.destroyed) return;

await EE.once(dest, 'drain');

}

Co-authored-by: Robert Nagy <[email protected]>

Ayase-252 added 2 commits April 27, 2021 22:47

fs: introduce filehandle.stream

5f71521

Fixes: nodejs#38350

fixup

f1f7ad8

github-actions bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Apr 27, 2021

ronag reviewed Apr 27, 2021

View reviewed changes

fixup

6b90fa5

ronag requested changes Apr 27, 2021

View reviewed changes

aduh95 reviewed Apr 27, 2021

View reviewed changes

benchmark/fs/bench-stream-via-pipeline.js Outdated Show resolved Hide resolved

jasnell reviewed Apr 27, 2021

View reviewed changes

lib/internal/fs/promises.js Outdated Show resolved Hide resolved

addaleax reviewed Apr 27, 2021

View reviewed changes

addaleax mentioned this pull request Apr 27, 2021

FileHandle.stream #38350

Closed

Ayase-252 added 6 commits April 28, 2021 20:04

fs: shrink when not fit and reallocate buffer on every iteration

1dc0168

fs: add validation for abort controller

65d4c46

fs: use internal/validator to validate parameters

a97a750

fs: change default value of start

cd6bf94

fs: improve benchmarking script

01d07cb

fs: validate dest

f7f6039

aduh95 reviewed Apr 28, 2021

View reviewed changes

jasnell reviewed Apr 28, 2021

View reviewed changes

lib/internal/fs/promises.js Outdated

end,

} = options;

if (start) {

Copy link

Member

jasnell Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

if (start) {

if (start !== undefined) {

jasnell reviewed Apr 28, 2021

View reviewed changes

fs: improve benchmark code

66535e8

Co-authored-by: Antoine du Hamel <[email protected]>

fs: improve parameter validation

47fb078

Co-authored-by: James M Snell <[email protected]>

ronag reviewed Apr 29, 2021

View reviewed changes

fs: improve compat with old streams

08ed687

Co-authored-by: Robert Nagy <[email protected]>

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

Ayase-252 closed this Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: introduce `filehandle.stream` #38440

fs: introduce `filehandle.stream` #38440

Ayase-252 commented Apr 27, 2021

ronag Apr 27, 2021

ronag Apr 27, 2021

ronag Apr 27, 2021 •

edited

Loading

wa-Nadoo commented Apr 27, 2021

aduh95 left a comment

aduh95 Apr 27, 2021

aduh95 Apr 27, 2021

jasnell Apr 27, 2021

ronag Apr 27, 2021

jasnell Apr 28, 2021

jasnell Apr 27, 2021

jasnell Apr 27, 2021

Mifrill Nov 20, 2023

addaleax Apr 27, 2021

addaleax Apr 27, 2021

Ayase-252 commented Apr 28, 2021

Ayase-252 commented Apr 28, 2021 •

edited

Loading

aduh95 Apr 28, 2021

aduh95 Apr 28, 2021

Ayase-252 Apr 28, 2021

jasnell Apr 28, 2021

Ayase-252 Apr 28, 2021

jasnell Apr 28, 2021

jasnell Apr 28, 2021

ronag Apr 29, 2021

Ayase-252 Apr 29, 2021

ronag Apr 29, 2021 •

edited

Loading

		async function cleanUp(i) {
		await Promise.all([fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)]);

	await Promise.all([fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)]);
	return [fsp.unlink(`${sourceName}-${i}`), fsp.unlink(`${destName}-${i}`)];

fs: introduce filehandle.stream #38440

fs: introduce filehandle.stream #38440

Conversation

Ayase-252 commented Apr 27, 2021

Benchmark

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

wa-Nadoo commented Apr 27, 2021

aduh95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ayase-252 commented Apr 28, 2021

Ayase-252 commented Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

fs: introduce `filehandle.stream` #38440

fs: introduce `filehandle.stream` #38440

ronag Apr 27, 2021 •

edited

Loading

Ayase-252 commented Apr 28, 2021 •

edited

Loading

ronag Apr 29, 2021 •

edited

Loading