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

feat(tar): support for async streams #746

Merged
merged 4 commits into from
May 24, 2022

Conversation

nathan-c
Copy link
Contributor

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

Relates to #223

I have taken a first pass at making TAR support async streams which will help with streaming to and from network streams. I have also tried to dramatically reduce memory allocations associated with reading from and writing to TAR's.

I haven't added new tests yet or really checked what the coverage is like but if you are happy with how it looks as it, I can do that.

Below are some benchmarks run before and after my changes:
Before:

Method Job Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
ReadTarInputStream Job-IFGTKW .NET Core 3.1 122.3 μs 1.71 μs 1.43 μs 33.2031 1.2207 - 1112144 B
ReadTarInputStreamAsync Job-IFGTKW .NET Core 3.1 1,081.9 μs 21.64 μs 58.50 μs 37.1094 0.9766 - 1227248 B
WriteTarOutputStream Job-IFGTKW .NET Core 3.1 50.54 μs 0.339 μs 0.283 μs 0.3052 - - 12008 B
WriteTarOutputStreamAsync Job-IFGTKW .NET Core 3.1 1,135.09 μs 22.460 μs 51.154 μs 3.9063 - - 126985 B

After:

Method Job Toolchain Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
ReadTarInputStream Job-KKAMPY .NET Core 3.1 221.3 μs 0.76 μs 0.64 μs - - - 952 B
ReadTarInputStreamAsync Job-KKAMPY .NET Core 3.1 227.1 μs 4.05 μs 3.79 μs - - - 1096 B
WriteTarOutputStream Job-KKAMPY .NET Core 3.1 149.7 μs 2.71 μs 2.54 μs - - - 449 B
WriteTarOutputStreamAsync Job-KKAMPY .NET Core 3.1 146.7 μs 0.67 μs 0.56 μs - - - 448 B

@codecov
Copy link

codecov bot commented Apr 11, 2022

Codecov Report

Merging #746 (23e5f27) into master (cc8dd78) will increase coverage by 0.27%.
The diff coverage is 77.25%.

@@            Coverage Diff             @@
##           master     #746      +/-   ##
==========================================
+ Coverage   73.83%   74.11%   +0.27%     
==========================================
  Files          69       71       +2     
  Lines        8355     8441      +86     
==========================================
+ Hits         6169     6256      +87     
+ Misses       2186     2185       -1     
Impacted Files Coverage Δ
src/ICSharpCode.SharpZipLib/Tar/TarInputStream.cs 63.20% <62.88%> (+7.84%) ⬆️
src/ICSharpCode.SharpZipLib/Tar/TarOutputStream.cs 67.83% <67.44%> (+0.90%) ⬆️
...rc/ICSharpCode.SharpZipLib/Core/ExactMemoryPool.cs 75.00% <75.00%> (ø)
src/ICSharpCode.SharpZipLib/Tar/TarBuffer.cs 73.12% <86.66%> (+6.45%) ⬆️
src/ICSharpCode.SharpZipLib/Tar/TarHeader.cs 80.86% <95.83%> (+1.37%) ⬆️
.../ICSharpCode.SharpZipLib/Core/StringBuilderPool.cs 100.00% <100.00%> (ø)
src/ICSharpCode.SharpZipLib/Tar/TarEntry.cs 67.79% <100.00%> (-1.50%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc8dd78...23e5f27. Read the comment docs.

@piksel
Copy link
Member

piksel commented Apr 11, 2022

Wow, for some reason I thought Stream.WriteAsync was not available in .NET 4.5...
This should make the async versions of Zip[In/Out]putStream much clearer.

Anyway, this mostly looks great, but it does lower the test coverage a bit.

Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

There should be some more tests with the async versions of the code, but otherwise it looks good. Great work!

@nathan-c
Copy link
Contributor Author

Thanks for the feedback! I will create some more tests to cover the changes

@nathan-c
Copy link
Contributor Author

Some extra tests have now been added.

@piksel piksel changed the title Feat/tar async feat(tar): support for async streams May 24, 2022
@piksel piksel merged commit d843d6d into icsharpcode:master May 24, 2022
@nathan-c nathan-c deleted the feat/tar-async branch May 24, 2022 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants