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.

Sign up for GitHub

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

Jump to bottom

Utilize fs.copyFileSync if available. #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Utilize fs.copyFileSync if available. #41

wants to merge 1 commit into from

Conversation

stefanpenner
Copy link
Contributor

@stefanpenner stefanpenner commented Jul 27, 2018 •
edited
Loading

Although node-copy-file-sync module exists, it uses more modern JS features. Which this library cannot yet support (without major version bump)

[fixes #34]

@stefanpenner
Copy link
Contributor Author

stefanpenner commented Jul 27, 2018 •
edited
Loading

@kanongil / @kellyselden / @lifeart et.al got any cycles to give this a try? See if this helps windows perf.

@stefanpenner
Utilize fs.copyFileSync if available.
a8e229f 
* Currently would only be utilized on windows
* available in node 8.5.x
* prefer Copy-On-Write if available
* good benchmarks yarnpkg/yarn#3290

Although node-copy-file-sync module exists, it uses more modern JS features. Which this library cannot yet support (without major version bump)
@stefanpenner stefanpenner mentioned this pull request Jul 27, 2018
Open
@kellyselden
Copy link

Sure. What would be the best way to test this other than running the test suite?

@stefanpenner
Copy link
Contributor Author

stefanpenner commented Jul 31, 2018 •
edited
Loading

@kellyselden

  • on a current checkout
  • run cold build and share the times
  • run rebuild and share the times
  • then update all copies of symlink-or-copy in your node_modules
  • disable symlinks
  • run cold build and share the times
  • run rebuild and share the times

I'll also try to put together some benchmarks this week.

@lifeart
Copy link
Contributor

lifeart commented Aug 2, 2018

@stefanpenner

no-symlink node 10 win 8.1

old first build 56337ms
old first build 47514ms
old first build 46859ms
old rebuild 15407ms
old rebuild 11573ms
old rebuild 11350ms

new first build  48338ms
new first build  51485ms
new first build  47898ms
new rebuild  12211ms
new rebuild  10830ms
new rebuild  11595ms


using-symlink node 10 win 8.1

old first build 37375ms
old first build 37517ms
old first build 36819ms
old rebuild 8208ms
old rebuild 8419ms
old rebuild 7828ms

new first build  36811ms
new first build  36631ms
new first build  36289ms
new rebuild  8514ms
new rebuild  8451ms
new rebuild  8617ms

@lifeart
Copy link
Contributor

lifeart commented Aug 2, 2018 •
edited
Loading

Master Branch

Tests failing without symlinks
image

Using symlinks - ok

image

FS-COPY-SYNC Branch

without symlinks

image

Using symlinks
image

@stefanpenner
Copy link
Contributor Author

I'll have to re-test this on new versions of OSX, but when tested last time it was a wash or worse for smallish files.

It may be the case that we perform the operation based on curtain heuristics?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use fs.copyFile() api in node 8.5+
3 participants
@stefanpenner @kellyselden @lifeart