Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

internal/fs: Don't Sync() destination file after copy #1408

Merged
merged 2 commits into from
Nov 28, 2017

Conversation

somersf
Copy link
Contributor

@somersf somersf commented Nov 28, 2017

When copying files, directly check for errors when closing
the destination file rather than using out.Sync() with
a deferred Close().

The out.Sync() call can be incredibly expensive on systems which
are I/O-heavy, yet may have more than ample memory and CPU
resources. As part of a file copy, we only need to ensure that
errors writing and closing the file handle are checked and dealt
with, rather than forcing the entire copied file to physical storage.

Environments such as Continuous Integration servers and container
hosts are often I/O-heavy. In such environments, this change
can make a significant difference - milliseconds vs minutes.

What does this do / why do we need it?

Avoid unnecessarily syncing newly copied files to physical storage.
A sync to physical disk imposes heavy latency on completing the file copy, and it is
exacerbated when it is done file-at-a-time.

Replacing the out.Sync() call with an error check on out.Close() can dramatically improve
latency on I/O-heavy machines, and actually improves error detection vs the out.Sync() call.

On a container host that I use for CI, this reduced a dep run from 6 minutes to 5 seconds.
(the host has copious RAM and CPU, but is heavily I/O bound)

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

When copying files, directly check for errors when closing
the destination file rather than using out.Sync() with
a deferred Close().

The out.Sync() call can be incredibly expensive on systems which
are I/O-heavy, yet may have more than ample memory and CPU
resources.  As part of a file copy, we only need to ensure that
errors writing and closing the file handle are checked and dealt
with, rather than forcing the entire copied file to physical storage.

Environments such as Continuous Integration servers and container
hosts are often I/O-heavy.  In such environments, this change
can make a significant difference - milliseconds vs minutes.
internal/fs: Don't Sync() destination file after copy
golang#1408
@sdboyer
Copy link
Member

sdboyer commented Nov 28, 2017

oh, yes! 🎉 🎉 we're not writing a database here - these calls don't need to guarantee data is flushed to disk prior to the function returning. the various failure modes that arise from not doing an fsync here are within our tolerances.

@sdboyer sdboyer merged commit f2d75a0 into golang:master Nov 28, 2017
@somersf somersf deleted the no-fsync branch November 28, 2017 17:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants