Closed Douglas-Cleghorn closed 1 year ago
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
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.