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

fix(ext/node): add fs.cp, fs.cpSync, promises.cp #21745

Merged
merged 11 commits into from
Jan 5, 2024

Conversation

littledivy
Copy link
Member

@littledivy littledivy commented Dec 31, 2023

Fixes #20803
Fixes #21723

Performance: copying a 48GiB rust target folder (recursive)

Platform deno node v21.5 Improvement
macOS (APFS) 3.1secs 127.99 secs 42x
Windows 18.3secs 67.2secs 3.8x

Copying files with varying sizes:

image

@littledivy littledivy marked this pull request as ready for review December 31, 2023 12:35
@@ -25,6 25,7 @@ fs3.workspace = true
libc.workspace = true
log.workspace = true
rand.workspace = true
rayon = "1.8.0"
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use spawn_blocking instead. Bringing rayon for these APIs seems like a lot

Copy link
Member Author

Choose a reason for hiding this comment

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

spawn_blocking cannot be used in non-async context.

Looking at Cargo.lock, rayon only adds crossbeam dependency. rayon is a little more suitable here because we want to parallelize large amounts of small file copies.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's land as is

@littledivy littledivy merged commit df062d2 into denoland:main Jan 5, 2024
14 checks passed
@littledivy littledivy deleted the node_cp branch January 7, 2024 04:26
bartlomieju pushed a commit that referenced this pull request Jan 12, 2024
Fixes #20803
Fixes #21723

Performance: copying a 48GiB rust `target` folder (recursive)
| Platform  | `deno` | `node v21.5` | Improvement |
| -------- | ------- | ------- | ------- |
| macOS (APFS) |   3.1secs  |  127.99 secs |  **42x** |
| Windows | 18.3secs | 67.2secs |  **3.8x** |

Copying files with varying sizes:


![image](https://github.com/denoland/deno/assets/34997667/58932652-6f7a-47f5-8504-896dc9ab4ddc)
@jcbhmr
Copy link
Contributor

jcbhmr commented Feb 2, 2024

I think you forgot to add it to https://github.com/littledivy/deno/blob/20bb2c8daaba6b78710a24014828f3db90e4e807/ext/node/polyfills/fs/promises.ts lol

This fails in 1.40.3

import { cp } from "node:fs/promises"
$ deno run -A z.js
error: Uncaught SyntaxError: The requested module 'node:fs/promises' does not provide an export named 'cp'
import { cp } from "node:fs/promises";
         ^
    at <anonymous> (file:///workspaces/myworkspace/z.js:1:10)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nuxt build fails with promises.cp is not a function node:fs cp support
3 participants