Skip to content

Support darwin clonefile#223

Merged
AkihiroSuda merged 2 commits into
containerd:mainfrom
dmcgowan:support-darwin-clonefile
May 17, 2023
Merged

Support darwin clonefile#223
AkihiroSuda merged 2 commits into
containerd:mainfrom
dmcgowan:support-darwin-clonefile

Conversation

@dmcgowan

@dmcgowan dmcgowan commented May 16, 2023

Copy link
Copy Markdown
Member

Adds clonefile support on copy for Darwin.

Added benchmark to show it is working

goos: darwin
goarch: amd64
pkg: github.com/containerd/continuity/fs
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
                  │   before.txt    │              after.txt              │
                  │     sec/op      │   sec/op     vs base                │
LargeCopy100MB-16    92802.9µ ± 11%   412.2µ ± 3%  -99.56% (p=0.000 n=10)
LargeCopy1GB-16     894137.8µ ± 11%   409.6µ ± 3%  -99.95% (p=0.000 n=10)
geomean                288.1m         410.9µ       -99.86%

                  │   before.txt   │              after.txt               │
                  │      B/op      │     B/op      vs base                │
LargeCopy100MB-16   38.913Ki ±  6%   3.961Ki ± 0%  -89.82% (p=0.000 n=10)
LargeCopy1GB-16     45.193Ki ± 18%   3.961Ki ± 0%  -91.24% (p=0.000 n=10)
geomean              41.94Ki         3.961Ki       -90.55%

                  │ before.txt │             after.txt              │
                  │ allocs/op  │ allocs/op   vs base                │
LargeCopy100MB-16   41.00 ± 0%   35.00 ± 0%  -14.63% (p=0.000 n=10)
LargeCopy1GB-16     42.50 ± 4%   35.00 ± 0%  -17.65% (p=0.000 n=10)
geomean             41.74        35.00       -16.15%

@dmcgowan
Add copy benchmark
ebdd069 
Signed-off-by: Derek McGowan <derek@mcg.dev>
samuelkarp
samuelkarp approved these changes May 16, 2023
View reviewed changes
Comment thread fs/copy_nondarwin.go

package fs

var copyFile = openAndCopyFile

@samuelkarp samuelkarp May 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not wild about a var here vs func copyFile(target, source string) error { return openAndCopyFile(target, source) } but since it's private it's a very minor concern.

@dmcgowan dmcgowan May 16, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah me either, it just seemed cleaner though. If there is an argument to be made that var is slower, then will update.

@dmcgowan dmcgowan mentioned this pull request May 17, 2023
Merged
AkihiroSuda
AkihiroSuda reviewed May 17, 2023
View reviewed changes
Comment thread fs/copy_darwin.go Outdated
@@ -0,0 +1,38 @@
//go:build darwin || freebsd || openbsd || netbsd || solaris
// +build darwin freebsd openbsd netbsd solaris

@AkihiroSuda AkihiroSuda May 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems wrong

@dmcgowan dmcgowan May 17, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah, those must have been ignored anyway, removed

AkihiroSuda
AkihiroSuda requested changes May 17, 2023
View reviewed changes

@AkihiroSuda AkihiroSuda left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build tags seem wrong, LGTM other than that

@dmcgowan
Add support for Darwin clonefile
30cf84d 
Signed-off-by: Derek McGowan <derek@mcg.dev>
@AkihiroSuda AkihiroSuda linked an issue May 17, 2023 that may be closed by this pull request
fs.CopyFile should use clonefile(2) on macOS when VOL_CAP_INT_CLONE is available #181
Closed
@AkihiroSuda

AkihiroSuda commented May 17, 2023

Copy link
Copy Markdown
Member

Let's release v0.4.0 after merging this

@dmcgowan dmcgowan force-pushed the support-darwin-clonefile branch from 31c4d57 to 30cf84d Compare May 17, 2023 06:17
@AkihiroSuda AkihiroSuda merged commit 21eb927 into containerd:main May 17, 2023
@AkihiroSuda

AkihiroSuda commented May 17, 2023

Copy link
Copy Markdown
Member

https://github.com/containerd/continuity/releases/tag/v0.4.0

@slonopotamus

slonopotamus commented Jun 27, 2023

Copy link
Copy Markdown

Nice! This makes native containerd snapshotter actually usable on macOS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samuelkarp samuelkarp samuelkarp approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@dcantah dcantah dcantah approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Pull Request Review
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fs.CopyFile should use clonefile(2) on macOS when VOL_CAP_INT_CLONE is available

5 participants

@dmcgowan @AkihiroSuda @slonopotamus @samuelkarp @dcantah