Elskom / zlib.managed

Based on http://www.componentace.com/zlib_.NET.htm
https://www.nuget.org/packages/zlib.managed/
MIT License
13 stars 9 forks source link

Benchmarks and Unit tests? #50

Open JimBobSquarePants opened 4 years ago

JimBobSquarePants commented 4 years ago

Hi, Sorry if this is not the best place to ask questions but I'm very curious about this library and how it compares in both performance and compression to other implementations.

Do you have unpublished benchmarks and unit tests to accompany the source code?

AraHaan commented 4 years ago

oh sorry I have not came around to benchmark or adding unit tests for this project atm. I was hoping of somehow auto generating unit tests for this like what rosyln does. However it seems IntelliTest is net framework only that auto generates unit tests. :sob: I think microsoft needs to add support for it for sdk style projects that targets frameworks like mine does.

For my XmlAbstraction package/project I use Xunit, however it seems Xunit does not like testing on netstandard2.0 for their visual studio adapter package, but on net40 the console and test adapter for vs resolves but the main Xunit package does not support it so I am ran across a cliff on needing to test all supported TargetFrameworks (for best code coverage to capture and dead or unused code that is #ifd based on specific TargetFrameworks).

I been planning on eventually adding unit tests and benchmarks for all of my packages though.

Sorry that I just now seen your issue, it somehow got lost in my notifications as I normally look at the ones I am participating in to see if anything updated.

Also because of that I been basically using this library in a ton of local projects to the point where I would know if something is wrong on this library from those, and one of those projects actually unpacks like about 12 GB worth of compressed data or so RIP. There was 1 time I screwed up something when I tried to fix a compile warning where all of the data would output at the correct unpacked length but all bytes would be null because every time it wanted me to use List instead of array and so every time it would create a new array (until I fixed that).

Oh wow you also work on xplat drawing stuff for net core, I am sure going to need that if I want to make an .net core only internet browser for windows, mac, and linux using the same code without any conditional windows junk. I know for sure I need in that case an html renderer of some kind, some way to render it to a window with tabs, theme the entire thing, js support, apng, all the other things normal browsers supports but also be super fast and more memory efficient than chrome, firefox, etc.

JimBobSquarePants commented 4 years ago

Hi @AraHaan no need to apologize. You're under no obligation to reply at all 😄

XUnit cannot run tests against net standard since it needs to run against an implementation of the standard, not the standard itself. By targeting multiple implementations you should be able to cover most things.

Regarding target frameworks, it looks like you target a lot of frameworks, some of which aren't even supported by Microsoft. From my experience it's best to target the least number of frameworks possible and only add when there are specific enhancements that can be added.

I see a dependency on some GitBuildInfo which is influencing your target framework count. There's better alternatives out there for determining build numbers that don't require including in the source. For example, in the ImageSharp codebase we use GitVersion

https://github.com/SixLabors/ImageSharp/blob/f6122dfe41ff8084e2bee2b9ad061beec542e99c/.github/workflows/build-and-test.yml#L63

As far as unit tests go I would probable use the Zlib tests from SharpZipLib and the DeflateStream tests from the dotnet runtime as inspiration.

https://github.com/icsharpcode/SharpZipLib/tree/f36ca373206340a25b1d3bb226acc7b679128a7c/test

https://github.com/dotnet/runtime/blob/1cfa461bbf071fbc71ceb5e105e1d39d0c077f25/src/libraries/System.IO.Compression/tests/CompressionStreamUnitTests.Deflate.cs

I'll have a deeper look when I can to see if I can get some tests added.

AraHaan commented 4 years ago

Ye, I made the GitBuildInfo package before to allow code to determine if they are using a dirty source tree or clean one, and to also determine if it is a master branch build or not as well.

JimBobSquarePants commented 4 years ago

Think I've found an issue with the inflater btw.

Roundtrip deflate/inflate fails on the inflater side with larger buffers, returning zeros after array[504] of a decompressed stream given a 4*4096 input buffer to compress/decompress. The original ZLib.NET implementation doesn't seem to have this issue but first 7 bytes of the expanded buffer appear to be incorrect in that implementation so it's hard to compare what is going on.

JimBobSquarePants commented 4 years ago

Actually, turns out the issue is present in both libraries.

https://github.com/Elskom/zlib.managed/blob/dbc4aa287abc874299a9201a93831d44dcd0ffb4/zlib.managed/ZInputStream.cs#L171

Should be

while (this.Z.AvailOut > 0 && err == ZlibCompressionState.ZOK); 
AraHaan commented 4 years ago

oh crud I see it now.

Btw, is the same issue in ZOutputStream or anywhere else too?

AraHaan commented 4 years ago

Alright I pushed the fix earlier.

AraHaan commented 4 years ago

It seems the pr adds 2 warnings to the build I would like resolved or silenced.