Octodiff icon indicating copy to clipboard operation
Octodiff copied to clipboard

Adding the ability to work with patches in memory.

Open Douglas-Cleghorn opened this issue 5 years ago • 1 comments

This PR adds the ability to chain patch files for handling incremental diffs without having to write the entire file to disk at each step. It also enables working with large files (>2GB) in memory.

I didn't see a good way to accept an unknown number of patch files for the command line version. If you can give me some pointers, I'll try to get that implemented. Otherwise, this will only work for the library version.

Douglas-Cleghorn avatar May 08 '19 17:05 Douglas-Cleghorn

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 08 '19 17:05 CLAassistant

Hi Douglas

Apologies on behalf of Octopus for taking so long to respond to this PR. We've recently re-assessed it and determined that it would be best not to merge.

While the ability to apply multiple patches without an interim file being written to disk is technically interesting, it does not satisfy any needs that we or our customers have at this time.

I also ran some performance tests and found that the BinaryDeltaStream is slower than the current implementation of DeltaApplier

Process: I checked out the code from your branch, bought it up to speed with our current Git Master, and used BenchmarkDotNet to run the same test through both our existing DeltaApplier and BinaryDeltaStream. The BinaryDeltaStream ends up using approximately the same amount of memory, but is measurably slower.

Small Delta

Method Mean Error StdDev Allocated
DeltaApplier.Apply 67.56 ms 1.766 ms 0.630 ms 178 MB
BinaryDeltaStream.Apply 73.20 ms 1.643 ms 0.427 ms 185.45 MB

Large Delta (I lost the memory allocation data for this, but it was approximately the same as the above test)

Method Mean Error StdDev
DeltaApplier.Apply 2,669.43 ms 1,019.144 ms 363.436 ms
BinaryDeltaStream.Apply 7,848.26 ms 202.927 ms 52.700 ms

As such, we think it'd better to keep OctoDiff as simple as reasonably possible, and close this PR.

If you disagree, and/or you have a reason why this ability would be valuable to you, could you please reply and help us understand? we can re-open the PR if need be.

Kind regards

Orion Edwards Senior Software Engineer at Octopus Deploy

borland avatar Jul 03 '23 03:07 borland