-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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: fsync: false when writing cache files #9695
Conversation
Hi @FauxFaux! Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
5146ec0
to
edccbd6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool, didn't know about this. Thanks!
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Some tests are failing since we have assertions about what the function is called with, updating the tests should be quick 👍 |
We're using writeFileAtomic here in order to get atomic rename, not to get full system power failure resilience. Most(tm) filesystems will guarantee either/or during a crash here, which is plenty enough for us. fsync is a nearly 3x slowdown for me, ts-jest'ing my whole codebase (~900 files in testcase).
edccbd6
to
4ef8aa7
Compare
Codecov Report
@@ Coverage Diff @@
## master #9695 +/- ##
=======================================
Coverage 65.09% 65.09%
=======================================
Files 287 287
Lines 12144 12144
Branches 3007 3009 +2
=======================================
Hits 7905 7905
Misses 3604 3604
Partials 635 635
Continue to review full report at Codecov.
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
writeFileAtomic
defaults to runningfsync
on the file after use.fsync
is a nearly 3x slowdown for me, ts-jest'ing my whole codebase (~900 files in testcase).ubuntu
defaults to/tmp
as regular filesystem (i.e. not tmpfs), and some filesystems/devices have a large overhead for these repeatedfsync
s. I happen to be on nvme (sm961), andbtrfs
.We're using
writeFileAtomic
here in order to get atomic rename, not to get full system power failure resilience. Most(tm) filesystems will guarantee either/or during a system power failure here, which is plenty enough for us.Test plan
There should be no functional changes here (even during a system power failure), only a performance increase in all cases, and a drastic performance increase in some cases.