-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
Add copy-on-write support when using MacOS. #160
Conversation
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.
First of all, thank you so much for your proposal. It's very interesting.
I reviewed your commits and a few thoughts came to mind.
- The CI is failing, which is not acceptable as in its current state.
- We shouldn't use
CopyInTest
as it doesn't represents users use-cases. We should avoid any special logic in the tests as much as possible. - Is it the responsibility of
copy
's (and its option's) to decide whether or not to usecopy-on-write
function? If the user is aware of usingcopy-on-write
, they should handle it with their code likeunix.Clonefile
. - What is
applyPermissionControl
? I don't think it is directly related to your goal. Making things too DRY here could be confusing. Let's aim to have one clear focus per pull request.
I really appreciate your contributions, and I'd like to discuss the third point of the list above in more detail.
Thanks for the quick response! To address the points you mentioned:
Don't worry, I'm not about to introduce changes that leave the CI in a failing state :) Looking at the failing tests, it appears that using two wrapped errors in
Fair. I would have preferred if it were possible to pass along a func withTestDefaults(opts... Option) []Option {
// Change the options here.
}
// And use it like:
err = Copy("src", "dest", withTestDefaults(Options { /* ... */ }))
In my opinion, To explain in a bit more detail, the goal is to create a perfect copy of a file. By directly using In contrast, the current implementation is using a
I do think users know what "copy-on-write" is as an idea2, but I don't think nearly as many of them are aware of how to actually use it in a program let alone on the command line3. Go's standard library also doesn't make it accessible as a simple function, which means that using the mechanism requires diving into OS-specific I also feel that a lot of the utility provided by the
A small side note: I would actually have preferred if the copy-on-write support I added in this pull request was opt out instead of opt in, but I didn't want to make such a significant change without your feedback or a
Fair point. I didn't have the full history behind
I refactored it into Footnotes
|
Thanks. I'll make my proposal on it and will ask you some comments on it. I don't want you to do double work so you can wait n see, though I don't stop you ;) |
If you're happy with the overall structure/design I came up with here, I'm happy to take your feedback into account and update my commits. It will save you the trouble of writing an alternate implementation :) |
I have alternative idea for interface design. |
Looking forward to seeing it! |
One thing to clarify. Do we really need fallback? |
Having a fallback is needed, but it doesn't have to be an option that the user can change. Unlike block copying, copy-on-write has a few extra requirements:
The fallback is needed in case of (1) and (2). |
Is there any case in which users CANNOT know the cases (1) and (2) beforehand? |
Technically, no. From a practical standpoint, I would say "yes". It's always possible to check for individual files: For (1), you can compare the For (2), there are It's not reasonable to expect that those always hold true for every file when it's true for the original source and destination directories, though. For example, a subdirectory under the source could be a mount point for a different filesystem, or the destination directory could already exist and have a different filesystem mounted to a subdirectory under it. |
After reading your version, I feel that some additional context around my implementation would be helpful: I was going for a transparent implementation where the user could "set and forget," so to speak. They would set the option, and Eventually, if you made a |
I'll make some changes to this after work today to simplify things. Hopefully you'll prefer that implementation over the one I made before talking with you 😄 |
Thank you. Don't rush, take your time. Looking forward your idea. Thanks again. FYI:
That's why most of Options are |
This series of commits adds support for using the clonefile syscall for copying regular files using APFS's copy-on-write mechanism.
It's implemented in a way that:
NeverCopyOnWrite
(default)CopyOnWritePreferred
— try to copy-on-write, fall back to copying contents if that failsCopyOnWriteRequired
— require copy-on-write to succeed when the platform is supported-tags=test_cow
flag for tests so it can run all the normal tests usingCopyOnWritePreferred
.test_cow
and without.This pull request doesn't include Linux support, as I don't currently have access to a Linux machine running btrfs.
It should be fairly straightforward to use the syscalls for it, though: https://stackoverflow.com/a/52799021
Benchmarks
Environment is a M1 MacBook Pro.
Part 1: Performance Impact for Non-CoW Usage
These Benchmarks are performed by running
go test -count=1
10 times.Benchmark @ origin/main
Baseline.
**Benchmark @ 6bf4915 **
Conclusion: With the extra abstraction, the difference is negligible. If anything, it's likely noise from inconsistent IO speeds or background processes.
Part 2: Performance Impact for CoW Usage
These benchmarks are performed by running
go test -run '^TestOptions_CopyOnWrite$' -count=1
10 times.With the file size being 32 MiB, these are the results:
**Benchmark @ 6b56d09 **
Baseline.
**Benchmark @ c97ba43 **
Conclusion: A small improvement for a 32 MiB file.
I also tested a 16 GiB file, and it goes down from 8.058s to 0.381s.