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

Clone files on OSX-like platforms when possible, instead of copying the whole file #79243

Merged
merged 85 commits into from
Jun 13, 2023

Conversation

hamarb123
Copy link
Contributor

@hamarb123 hamarb123 commented Dec 5, 2022

Fixes #77835.

This should improve performance greatly for File.Copy when copying from one APFS volume to a different file in the volume (I think it's within a volume anyway). It will clone the file which is instant instead of copying all of the bytes, and if cloning doesn't work it will fall back to the normal method of copying.

Informative:

  • Uses clonefile when possible on macOS to clone the file while still keeping file locking logic intact
  • Split common Unix logic into multiple functions that the macOS implementation uses parts of at different times
  • No shim needed since the ABI will not change. Precedent for doing it this way for macOS-specific functions: Fix setting creation date for OSX-like platforms #49555
  • Applies to all OSX-like platforms (macOS, iOS, tvOS, and Mac Catalyst), not just macOS

Todo:

  • Need to add tests to check the file is actually cloned so we know if it works or not
  • The logic in this commit has not been tested thoroughly (but it has been tested somewhat), so will wait to see the GitHub actions failures before refactoring (if needed)
  • Do we want tests copying between different "drives"? I think these would be valuable.

• Use copyfile (with COPYFILE_CLONE_FORCE) when possible on macOS to clone the file while still keeping file locking logic intact
• Split common Unix logic into multiple functions that the macOS implementation uses parts of at different times
• Add string version of ResolveLinkTarget to save the allocation since part of the code needs it
• Need to add tests to check the file is actually cloned so we know if it works or not
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 5, 2022
@ghost
Copy link

ghost commented Dec 5, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #77835.

This should improve performance greatly for File.Copy when copying from one APFS volume to a different file in the volume (I think it's within a volume anyway). It will clone the file which is instant instead of copying all of the bytes, and if cloning doesn't work it will fall back to the normal method of copying.

Informative:

  • Uses copyfile (with COPYFILE_CLONE_FORCE) when possible on macOS to clone the file while still keeping file locking logic intact
  • Split common Unix logic into multiple functions that the macOS implementation uses parts of at different times
  • No shim needed since the ABI will not change. Precedent for doing it this way for macOS-specific functions: Fix setting creation date for OSX-like platforms #49555
  • Applies to all OSX-like platforms, not just macOS

Todo:

  • Need to add tests to check the file is actually cloned so we know if it works or not
  • The logic in this commit has not been tested thoroughly (but it has been tested somewhat), so will wait to see the GitHub actions issues before refactoring (if needed)
  • Is it a problem that it could theoretically delete the destination file and then fail for some other reason, leaving the destination file deleted when it may not have done so before (realistically this is extremely unlikely)?
Author: hamarb123
Assignees: -
Labels:

area-System.IO

Milestone: -

@hamarb123
Copy link
Contributor Author

Btw, this implementation is not done, I want to see if any tests fail first though.

• I missed setting StringMarshalling on the LibraryImport
• I missed partial on the CopyFile method implementations
• Apparently 'Both partial method declarations must be unsafe or neither may be unsafe'
• I love partial methods
• Move unsafe keyword into the OSX method rather than in method declaration
• Fix accessibility modifier of CopyFile in FileSystem.Unix.cs
• Fix indentation (should have used spaces)
• Import Microsoft.Win32.SafeHandles in FileSystem.CopyFile.OSX.cs file
• Fix for SA1205 'Partial elements should declare an access modifier'
• Fix typo in FileSystem.CopyFile.OtherUnix.cs of 'startedCopyFile'
• Fix missing openDst parameter code in StartCopyFile
• Fix not checking the nullability in StandardCopyFile, which led to 'Possible null reference argument for parameter'
• Add missing parameter value (openNewFile) to OpenCopyFileDstHandle
• Fix misnamed variable usages throughout some code
• Properly qualify Interop.ErrorInfo
• This should be the last fix for build issues for this set
@hamarb123
Copy link
Contributor Author

Sorry for all of the fix commits, I couldn't get build locally to work, so I copied the code into a new project to test it and must have missed some of the changes here.

@hamarb123 hamarb123 marked this pull request as draft December 5, 2022 21:44
• Add missing nullable ? for dstHandle (which obtains the file lock)
• Test failures were since copying a file onto itself should cause an error, this fixes that
…ain)

• The variable in the if statements weren't updated
• Handling for if readlink fails
…t#77835'

• The source path should have used sourceFullPath rather than destPath
@marek-safar marek-safar added the os-mac-os-x macOS aka OSX label Dec 6, 2022
@hamarb123
Copy link
Contributor Author

hamarb123 commented Dec 6, 2022

@marek-safar, this applies to macOS, iOS, tvOS, and Mac Catalyst in case you wanted to add tags for those. Can you also 'assign' me if you're able? Thanks.

@hamarb123 hamarb123 changed the title [WIP] Cloning files on macOS when possible instead of copying the whole file [WIP] Cloning files on OSX-like platforms when possible instead of copying the whole file Dec 6, 2022
@ghost ghost closed this Jan 6, 2023
@ghost
Copy link

ghost commented Jan 6, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@adamsitnik
Copy link
Member

@filipnavara is there any chance you could review this PR? You are currently the most experience person in this particular area.

@hamarb123
Copy link
Contributor Author

Is there a reason why 24 test runs have been cancelled so far? Do the tests need to be re-run?

@danmoseley
Copy link
Member

@hamarb123 it is dotnet/arcade#13774

@slang25
Copy link
Contributor

slang25 commented Jun 12, 2023

I'm really looking forward to this change! Will we be seeing it land in a .NET 8 preview?

@danmoseley
Copy link
Member

I'm really looking forward to this change! Will we be seeing it land in a .NET 8 preview?

If it's merged in the next month or two, it will be in .NET 8.

@hamarb123
Copy link
Contributor Author

I'm really looking forward to this change! Will we be seeing it land in a .NET 8 preview?

It's hopefully just a few more stages of review at most. I'm hoping it will make it to .NET 8 also.

@tmds
Copy link
Member

tmds commented Jun 13, 2023

Per @adamsitnik's earlier comment, macOS isn't benchmarked in the perf lab.

So before merging, we need some benchmark results that verify there is a performance gain.

@hamarb123 can you look at the documentation Adam shared, and run the benchmarks on your machine?

@hamarb123
Copy link
Contributor Author

hamarb123 commented Jun 13, 2023

Benchmark results:

  • CopyTo is ~1.9x faster for small files with the changes, and is usually ~180us for all the tests up to 1MB (~207us for 100MB test). Note this test also deletes the file as a part of the test, so simply copying would likely be faster again.
  • CopyToOverwrite is ~1.15x faster for small files with the changes, and is ~215us for all the tests.
  • Copying tests in System.IO.Tests.Perf_FileStream did not improve (haven't included them here), do we want to work out why and improve that API also?
  • The comparison was between the commit just before this comment which merges all of the new runtime changes, and the runtime repo with those same runtime changes but without this PR.

System.IO.Tests.Perf_File

With changes
Method size Mean Error StdDev Median Min Max Allocated
CopyTo 512 180.4 us 3.42 us 3.66 us 180.0 us 173.9 us 186.3 us 65 B
CopyToOverwrite 512 217.4 us 4.17 us 4.10 us 217.2 us 210.2 us 225.9 us 129 B
CopyTo 4096 175.9 us 3.41 us 3.19 us 175.0 us 171.2 us 182.7 us 65 B
CopyToOverwrite 4096 214.0 us 2.41 us 2.13 us 213.4 us 209.8 us 218.2 us 129 B
CopyTo 1048576 175.9 us 2.41 us 2.01 us 175.9 us 173.4 us 179.8 us 65 B
CopyToOverwrite 1048576 212.3 us 3.97 us 3.31 us 212.0 us 208.1 us 218.1 us 129 B
CopyTo 104857600 206.9 us 2.32 us 1.94 us 206.6 us 203.7 us 210.8 us 65 B
CopyToOverwrite 104857600 213.9 us 3.58 us 3.17 us 214.2 us 209.1 us 218.0 us 129 B
Without changes
Method size Mean Error StdDev Median Min Max Allocated
CopyTo 512 347.6 us 12.46 us 14.35 us 344.8 us 326.4 us 374.2 us 129 B
CopyToOverwrite 512 249.2 us 5.56 us 6.18 us 248.7 us 241.4 us 261.1 us 129 B
CopyTo 4096 347.1 us 7.16 us 7.36 us 345.2 us 335.4 us 364.5 us 129 B
CopyToOverwrite 4096 244.6 us 3.07 us 2.72 us 243.9 us 240.4 us 248.4 us 129 B
CopyTo 1048576 1,313.5 us 68.13 us 72.90 us 1,322.9 us 1,191.9 us 1,437.3 us 133 B
CopyToOverwrite 1048576 1,773.5 us 321.63 us 370.39 us 1,730.5 us 1,184.8 us 2,432.6 us 136 B
CopyTo 104857600 55,961.7 us 4,954.14 us 5,705.19 us 53,346.9 us 49,530.1 us 65,915.7 us -
CopyToOverwrite 104857600 55,883.4 us 5,472.86 us 6,302.55 us 54,004.8 us 46,797.4 us 66,152.4 us -

@tmds
Copy link
Member

tmds commented Jun 13, 2023

CopyToOverwrite is ~1.15x faster for small files with the changes

That's a pleasant surprise.

My expectation was to see a regression for small files with overwrite since we're making additional system calls and the benefit of cloning is low for small files.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

The code looks very good, I found only one thing that could be improved (PTAL at my comment).

The perf win is very impressive. I've benchmarked it on my old macBook:

BenchmarkDotNet=v0.13.2.2052-nightly, OS=macOS Monterey 12.6.5 (21G531) [Darwin 21.6.0]
Intel Core i7-5557U CPU 3.10GHz (Broadwell), 1 CPU, 4 logical and 2 physical cores
.NET SDK=8.0.100-preview.6.23312.5
  [Host] : .NET 8.0.0 (8.0.23.30704), X64 RyuJIT AVX2
  PR     : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
  main   : .NET 8.0.0 (8.0.23.30704), X64 RyuJIT AVX2
Method Job size Mean StdDev Ratio
CopyTo PR 512 438.7 μs 106.58 μs 0.75
CopyTo main 512 588.0 μs 15.62 μs 1.00
CopyToOverwrite PR 512 298.8 μs 6.18 μs 0.79
CopyToOverwrite main 512 381.4 μs 4.55 μs 1.00
CopyTo PR 4096 237.0 μs 6.29 μs 0.39
CopyTo main 4096 606.9 μs 37.95 μs 1.00
CopyToOverwrite PR 4096 293.7 μs 7.59 μs 0.76
CopyToOverwrite main 4096 388.1 μs 3.02 μs 1.00
CopyTo PR 1048576 239.9 μs 10.95 μs 0.09
CopyTo main 1048576 2,567.9 μs 107.61 μs 1.00
CopyToOverwrite PR 1048576 298.2 μs 11.47 μs 0.12
CopyToOverwrite main 1048576 2,585.2 μs 63.77 μs 1.00
CopyTo PR 104857600 247.0 μs 8.00 μs 0.001
CopyTo main 104857600 230,797.9 μs 42,923.74 μs 1.000
CopyToOverwrite PR 104857600 291.4 μs 6.45 μs 0.001
CopyToOverwrite main 104857600 256,931.8 μs 2,784.23 μs 1.000

Great job @hamarb123 !!

@stephentoub
Copy link
Member

stephentoub commented Jun 13, 2023

The perf win is very impressive.

While this isn't a common scenario, in the name of completeness, what does the perf look like if, after copying, the benchmark then modifies the destination file in some way?

@adamsitnik
Copy link
Member

While this isn't a common scenario, in the name of completeness, what does the perf look like if, after copying, the benchmark then modifies the destination file in some way?

That is a very good question!

I've modified the benchmarks and just added File.WriteAllBytes(dest, File.ReadAllBytes(source)) right after the call to copy:

[Benchmark]
[Arguments(HalfKibibyte)]
[Arguments(FourKibibytes)]
[Arguments(OneMibibyte)]
[Arguments(HundredMibibytes)]
public void CopyTo_AndModify(int size)
{
    File.Delete(_testFilePath);
    File.Copy(_filesToRead[size], _testFilePath); // overwrite defaults to false
+   File.WriteAllBytes(_testFilePath, File.ReadAllBytes(_filesToRead[size]));
}

[Benchmark]
[Arguments(HalfKibibyte)]
[Arguments(FourKibibytes)]
[Arguments(OneMibibyte)]
[Arguments(HundredMibibytes)]
public void CopyToOverwrite_AndModify(int size)
{
    File.Copy(_filesToRead[size], _testFilePath, overwrite: true);
+   File.WriteAllBytes(_testFilePath, File.ReadAllBytes(_filesToRead[size]));
}

We have still a major win for large files and no regression for smaller files

Method Job size Mean StdDev Ratio
CopyTo_AndModify PR 512 533.1 μs 16.83 μs 0.71
CopyTo_AndModify main 512 742.4 μs 6.98 μs 1.00
CopyToOverwrite_AndModify PR 512 588.8 μs 25.28 μs 0.98
CopyToOverwrite_AndModify main 512 593.9 μs 10.35 μs 1.00
CopyTo_AndModify PR 4096 540.8 μs 15.53 μs 0.71
CopyTo_AndModify main 4096 767.0 μs 13.17 μs 1.00
CopyToOverwrite_AndModify PR 4096 590.7 μs 19.49 μs 1.00
CopyToOverwrite_AndModify main 4096 594.7 μs 5.73 μs 1.00
CopyTo_AndModify PR 1048576 2,383.3 μs 84.83 μs 0.64
CopyTo_AndModify main 1048576 3,701.9 μs 82.21 μs 1.00
CopyToOverwrite_AndModify PR 1048576 2,358.2 μs 69.96 μs 0.69
CopyToOverwrite_AndModify main 1048576 3,412.3 μs 87.54 μs 1.00
CopyTo_AndModify PR 104857600 167,426.8 μs 18,209.30 μs 0.61
CopyTo_AndModify main 104857600 276,862.8 μs 29,743.99 μs 1.00
CopyToOverwrite_AndModify PR 104857600 170,189.9 μs 26,538.74 μs 0.61
CopyToOverwrite_AndModify main 104857600 287,618.2 μs 48,952.24 μs 1.00

@stephentoub
Copy link
Member

File.WriteAllBytes

WriteAllBytes opens the file as Create, which means it's going to effectively delete the destination and write a new file rather than modify the one that's there.

@adamsitnik
Copy link
Member

WriteAllBytes opens the file as Create, which means it's going to effectively delete the destination and write a new file rather than modify the one that's there.

Sorry, I forgot about that (BTW it just proves that we should really add #84532)

Updated benchmark:

private byte[] _oneKb = new byte[1024];

[Benchmark]
[Arguments(HalfKibibyte)]
[Arguments(FourKibibytes)]
[Arguments(OneMibibyte)]
[Arguments(HundredMibibytes)]
public void CopyTo_AndModify(int size)
{
    File.Delete(_testFilePath);
    File.Copy(_filesToRead[size], _testFilePath); // overwrite defaults to false
    
    using FileStream fs = File.OpenWrite(_testFilePath);
    fs.Write(_oneKb);
}

[Benchmark]
[Arguments(HalfKibibyte)]
[Arguments(FourKibibytes)]
[Arguments(OneMibibyte)]
[Arguments(HundredMibibytes)]
public void CopyToOverwrite_AndModify(int size)
{
    File.Copy(_filesToRead[size], _testFilePath, overwrite: true);

    using FileStream fs = File.OpenWrite(_testFilePath);
    fs.Write(_oneKb);
}

And the results:

BenchmarkDotNet=v0.13.2.2052-nightly, OS=macOS Monterey 12.6.5 (21G531) [Darwin 21.6.0]
Intel Core i7-5557U CPU 3.10GHz (Broadwell), 1 CPU, 4 logical and 2 physical cores
.NET SDK=8.0.100-preview.6.23312.5
  [Host] : .NET 8.0.0 (8.0.23.30704), X64 RyuJIT AVX2
  PR     : .NET 8.0.0 (42.42.42.42424), X64 RyuJIT AVX2
  main   : .NET 8.0.0 (8.0.23.30704), X64 RyuJIT AVX2

IterationCount=40
Method Job size Mean StdDev Ratio
CopyTo_AndModify PR 512 537.7 μs 13.18 μs 0.82
CopyTo_AndModify main 512 661.3 μs 9.98 μs 1.00
CopyToOverwrite_AndModify PR 512 549.1 μs 16.54 μs 1.15
CopyToOverwrite_AndModify main 512 476.3 μs 7.58 μs 1.00
CopyTo_AndModify PR 4096 669.8 μs 20.57 μs 1.01
CopyTo_AndModify main 4096 662.4 μs 11.60 μs 1.00
CopyToOverwrite_AndModify PR 4096 695.3 μs 26.41 μs 1.50
CopyToOverwrite_AndModify main 4096 465.0 μs 4.94 μs 1.00
CopyTo_AndModify PR 1048576 722.5 μs 14.36 μs 0.35
CopyTo_AndModify main 1048576 2,051.8 μs 40.83 μs 1.00
CopyToOverwrite_AndModify PR 1048576 728.4 μs 34.28 μs 0.37
CopyToOverwrite_AndModify main 1048576 1,991.1 μs 163.53 μs 1.00
CopyTo_AndModify PR 104857600 804.9 μs 18.60 μs 0.007
CopyTo_AndModify main 104857600 109,415.3 μs 8,542.98 μs 1.000
CopyToOverwrite_AndModify PR 104857600 762.1 μs 26.40 μs 0.007
CopyToOverwrite_AndModify main 104857600 117,399.2 μs 16,068.51 μs 1.000

@adamsitnik adamsitnik merged commit 8fb25a8 into dotnet:main Jun 13, 2023
@adamsitnik adamsitnik added this to the 8.0.0 milestone Jun 13, 2023
@stephentoub
Copy link
Member

BTW it just proves that we should really add #84532

Not really 😄

Updated benchmark

Thanks. So this does regress small overwrites that are subsequently modified.

@adamsitnik
Copy link
Member

Not really

Why? Do you believe that appending a buffer of bytes to an existing file is a rare scenario?

@danmoseley
Copy link
Member

Thanks for this nice change @hamarb123 . Would you like to tackle another issue? There are also a number marked api-approved that are available.

@stephentoub
Copy link
Member

stephentoub commented Jun 13, 2023

BTW it just proves that we should really add #84532

Not really

Why? Do you believe that appending a buffer of bytes to an existing file is a rare scenario?

I'm not commenting on the scenario of appending a buffer of bytes to an existing file. I'm commenting that a need to mutate a recently copied file as part of benchmarking a copy-on-write optimization and to achieve that writing two short lines of code doesn't prove that we need to add a new API.

@hamarb123
Copy link
Contributor Author

Thanks for this nice change @hamarb123 . Would you like to tackle another issue? There are also a number marked api-approved that are available.

Thanks! I'll have a look tomorrow (also known as a today at a sensible hour lol). I was also going to do the one for ReFS on Windows, was setting up a VM to test dev drives yesterday.

Copying tests in System.IO.Tests.Perf_FileStream did not improve (haven't included them here), do we want to work out why and improve that API also?

Just following up on this also. Presumably this API just does a block copy with the files. Is this something that's commonly used (I know I've never done it like this)? Because it doesn't seem to benefit from this, but maybe no-one uses it, idk.

@adamsitnik
Copy link
Member

Copying tests in System.IO.Tests.Perf_FileStream did not improve

IIRC The FileStream.CopyTo(Stream destination) has no special handling for cases where the destination stream is also a file stream.

Is this something that's commonly used

I don't have enough data to answer your question, but if you can provide an optimization that improves such scenario without introducing a lot of code complexity I am happy to review it.

@danmoseley
Copy link
Member

I was also going to do the one for ReFS on Windows

Ah yes, indeed that would be nice to get into .NET 8

@ghost ghost locked as resolved and limited conversation to collaborators Jul 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member os-mac-os-x macOS aka OSX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File.Copy: Clone file when possible on MacOS
10 participants