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

faster copy-dereference to output #5370

Merged
merged 2 commits into from Jan 26, 2016
Merged

faster copy-dereference to output #5370

merged 2 commits into from Jan 26, 2016

Conversation

stefanpenner
Copy link
Contributor

@jonnii
Copy link
Contributor

jonnii commented Jan 17, 2016

@stefanpenner what does this mean for build times? (out of interest).

@stefanpenner
Copy link
Contributor Author

@stefanpenner what does this mean for build times?

This is a nice improvement for users with a large number of files in dist, as it now only applies a "patch".
For normal apps, there appears to be no noticeable difference. for apps with many medium to large size files, more dramatic improvements exist. In my test, for 5000 photos, taking 3gb of disk the rebuild overhead went from 5s -> 23ms.

TL;DR it should be net positive, as it preforms a similar number of disk operations for testing what files are available, but typically skips nearly all file reading and writing (which turns out to be kinda slow).

@jonnii
Copy link
Contributor

jonnii commented Jan 17, 2016

@stefanpenner interesting thanks!

@rwjblue
Copy link
Member

rwjblue commented Jan 18, 2016

The changes here look good, do you think we need any additional tests here (I'm not sure if we do)?

@stefanpenner
Copy link
Contributor Author

do you think we need any additional tests here (I'm not sure if we do)?

I don't think so, the observable difference is performance. Which can be a-bit difficult to test, and the observed differences are thoroughly tested in the new module. So we should be covered.

If issues occur, it is most likely going to be in the foreign module, and further tests will then need to be added there, rather then here.

@stefanpenner
Copy link
Contributor Author

this has some bugs, will investigate..

@stefanpenner
Copy link
Contributor Author

Turns out, I was accidentally re-initializing the sync object each time... silly me.

@stefanpenner
Copy link
Contributor Author

@homu r+

@homu
Copy link
Contributor

homu commented Jan 26, 2016

📌 Commit e50b191 has been approved by stefanpenner

@homu
Copy link
Contributor

homu commented Jan 26, 2016

⚡ Test exempted - status

@homu homu merged commit e50b191 into master Jan 26, 2016
homu added a commit that referenced this pull request Jan 26, 2016
@stefanpenner stefanpenner deleted the faster-copy-dereference branch January 26, 2016 07:41
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.

None yet

5 participants