oleg-st / ZstdSharp

Port of zstd compression library to c#
MIT License
200 stars 29 forks source link

Cleanup code and improve reliability. #38

Closed teo-tsirpanis closed 2 weeks ago

teo-tsirpanis commented 2 weeks ago

This PR:

oleg-st commented 2 weeks ago

1) The removal of the InlineIL and InlineMethod dependencies should depend on the benchmark results. I think InlineIL can be removed and if there are problems in older frameworks, we can leave the InlineIL dependency and the old implementation for them. It is more difficult to remove InlineMethod, because even .NET 8 benefits from its use. I created InlineMethod.Fody because the JIT inliner wasn't working well enough. 2) SafeHandle change LGTM overall with some comments. 3) I'm not removing support for older frameworks yet as long as it costs almost nothing to support them. The code contains framework-dependent optimizations.

teo-tsirpanis commented 2 weeks ago

Oh you wrote InlineMethod, nice! And I hadn't seen that there are optimizations for all targeted frameworks.

One advantage to targeting less frameworks is reduced package size; right now it is in 1.15MB, which is not yet but close to what I would call "big". But it's not the end of the world.

All feedback was addressed, and I also fixed a memory leak I spotted. I am going to run some benchmarks and will get back to you.

teo-tsirpanis commented 2 weeks ago

The results are in.

Before

``` BenchmarkDotNet v0.13.12, Windows 10 (10.0.19045.4598/22H2/2022Update) Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores .NET SDK 9.0.100-preview.4.24267.66 [Host] : .NET 8.0.6 (8.0.624.26715), X64 RyuJIT AVX2 .NET 5.0 : .NET 5.0.17 (5.0.1722.21314), X64 RyuJIT AVX2 .NET 6.0 : .NET 6.0.31 (6.0.3124.26714), X64 RyuJIT AVX2 .NET 7.0 : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2 .NET 8.0 : .NET 8.0.6 (8.0.624.26715), X64 RyuJIT AVX2 .NET Core 3.1 : .NET Core 3.1.30 (CoreCLR 4.700.22.47601, CoreFX 4.700.22.47602), X64 RyuJIT AVX2 .NET Framework 4.6.2 : .NET Framework 4.8.1 (4.8.9241.0), X64 RyuJIT VectorSize=256 ``` | Method | Job | Runtime | Mean | Error | StdDev | Ratio | RatioSD | |----------------- |--------------------- |--------------------- |---------:|---------:|---------:|------:|--------:| | CompressNative | .NET 5.0 | .NET 5.0 | 44.01 ms | 0.428 ms | 0.401 ms | 1.00 | 0.00 | | CompressSharp | .NET 5.0 | .NET 5.0 | 65.62 ms | 0.503 ms | 0.446 ms | 1.49 | 0.01 | | | | | | | | | | | CompressNative | .NET 6.0 | .NET 6.0 | 42.91 ms | 0.753 ms | 0.704 ms | 1.00 | 0.00 | | CompressSharp | .NET 6.0 | .NET 6.0 | 64.22 ms | 0.726 ms | 0.679 ms | 1.50 | 0.03 | | | | | | | | | | | CompressNative | .NET 7.0 | .NET 7.0 | 42.35 ms | 0.443 ms | 0.393 ms | 1.00 | 0.00 | | CompressSharp | .NET 7.0 | .NET 7.0 | 55.17 ms | 0.602 ms | 0.503 ms | 1.30 | 0.02 | | | | | | | | | | | CompressNative | .NET 8.0 | .NET 8.0 | 41.84 ms | 0.274 ms | 0.228 ms | 1.00 | 0.00 | | CompressSharp | .NET 8.0 | .NET 8.0 | 52.55 ms | 0.796 ms | 0.744 ms | 1.26 | 0.02 | | | | | | | | | | | CompressNative | .NET Core 3.1 | .NET Core 3.1 | 41.64 ms | 0.477 ms | 0.423 ms | 1.00 | 0.00 | | CompressSharp | .NET Core 3.1 | .NET Core 3.1 | 62.23 ms | 0.845 ms | 0.749 ms | 1.49 | 0.02 | | | | | | | | | | | CompressNative | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 41.57 ms | 0.532 ms | 0.444 ms | 1.00 | 0.00 | | CompressSharp | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 67.98 ms | 0.637 ms | 0.565 ms | 1.64 | 0.02 | | | | | | | | | | | DecompressNative | .NET 5.0 | .NET 5.0 | 10.85 ms | 0.170 ms | 0.159 ms | 1.00 | 0.00 | | DecompressSharp | .NET 5.0 | .NET 5.0 | 16.63 ms | 0.247 ms | 0.206 ms | 1.53 | 0.03 | | | | | | | | | | | DecompressNative | .NET 6.0 | .NET 6.0 | 10.70 ms | 0.135 ms | 0.119 ms | 1.00 | 0.00 | | DecompressSharp | .NET 6.0 | .NET 6.0 | 16.81 ms | 0.139 ms | 0.116 ms | 1.57 | 0.02 | | | | | | | | | | | DecompressNative | .NET 7.0 | .NET 7.0 | 10.71 ms | 0.118 ms | 0.098 ms | 1.00 | 0.00 | | DecompressSharp | .NET 7.0 | .NET 7.0 | 14.45 ms | 0.178 ms | 0.158 ms | 1.35 | 0.02 | | | | | | | | | | | DecompressNative | .NET 8.0 | .NET 8.0 | 10.75 ms | 0.161 ms | 0.150 ms | 1.00 | 0.00 | | DecompressSharp | .NET 8.0 | .NET 8.0 | 11.49 ms | 0.117 ms | 0.109 ms | 1.07 | 0.02 | | | | | | | | | | | DecompressNative | .NET Core 3.1 | .NET Core 3.1 | 10.70 ms | 0.165 ms | 0.154 ms | 1.00 | 0.00 | | DecompressSharp | .NET Core 3.1 | .NET Core 3.1 | 16.05 ms | 0.270 ms | 0.253 ms | 1.50 | 0.03 | | | | | | | | | | | DecompressNative | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 10.68 ms | 0.138 ms | 0.115 ms | 1.00 | 0.00 | | DecompressSharp | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 17.23 ms | 0.185 ms | 0.173 ms | 1.61 | 0.02 |

After

``` BenchmarkDotNet v0.13.12, Windows 10 (10.0.19045.4598/22H2/2022Update) Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores .NET SDK 9.0.100-preview.4.24267.66 [Host] : .NET 8.0.6 (8.0.624.26715), X64 RyuJIT AVX2 .NET 5.0 : .NET 5.0.17 (5.0.1722.21314), X64 RyuJIT AVX2 .NET 6.0 : .NET 6.0.31 (6.0.3124.26714), X64 RyuJIT AVX2 .NET 7.0 : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2 .NET 8.0 : .NET 8.0.6 (8.0.624.26715), X64 RyuJIT AVX2 .NET Core 3.1 : .NET Core 3.1.30 (CoreCLR 4.700.22.47601, CoreFX 4.700.22.47602), X64 RyuJIT AVX2 .NET Framework 4.6.2 : .NET Framework 4.8.1 (4.8.9241.0), X64 RyuJIT VectorSize=256 ``` | Method | Job | Runtime | Mean | Error | StdDev | Ratio | RatioSD | |----------------- |--------------------- |--------------------- |---------:|---------:|---------:|------:|--------:| | CompressNative | .NET 5.0 | .NET 5.0 | 48.66 ms | 0.956 ms | 1.063 ms | 1.00 | 0.00 | | CompressSharp | .NET 5.0 | .NET 5.0 | 69.90 ms | 1.387 ms | 2.240 ms | 1.46 | 0.04 | | | | | | | | | | | CompressNative | .NET 6.0 | .NET 6.0 | 43.01 ms | 0.699 ms | 0.654 ms | 1.00 | 0.00 | | CompressSharp | .NET 6.0 | .NET 6.0 | 63.50 ms | 0.585 ms | 0.519 ms | 1.47 | 0.03 | | | | | | | | | | | CompressNative | .NET 7.0 | .NET 7.0 | 42.28 ms | 0.638 ms | 0.597 ms | 1.00 | 0.00 | | CompressSharp | .NET 7.0 | .NET 7.0 | 55.06 ms | 0.985 ms | 0.922 ms | 1.30 | 0.02 | | | | | | | | | | | CompressNative | .NET 8.0 | .NET 8.0 | 41.82 ms | 0.215 ms | 0.180 ms | 1.00 | 0.00 | | CompressSharp | .NET 8.0 | .NET 8.0 | 51.86 ms | 0.579 ms | 0.541 ms | 1.24 | 0.02 | | | | | | | | | | | CompressNative | .NET Core 3.1 | .NET Core 3.1 | 41.65 ms | 0.407 ms | 0.381 ms | 1.00 | 0.00 | | CompressSharp | .NET Core 3.1 | .NET Core 3.1 | 59.98 ms | 1.148 ms | 1.074 ms | 1.44 | 0.03 | | | | | | | | | | | CompressNative | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 41.53 ms | 0.513 ms | 0.455 ms | 1.00 | 0.00 | | CompressSharp | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 70.03 ms | 0.406 ms | 0.380 ms | 1.69 | 0.02 | | | | | | | | | | | DecompressNative | .NET 5.0 | .NET 5.0 | 10.63 ms | 0.183 ms | 0.171 ms | 1.00 | 0.00 | | DecompressSharp | .NET 5.0 | .NET 5.0 | 16.88 ms | 0.237 ms | 0.210 ms | 1.59 | 0.04 | | | | | | | | | | | DecompressNative | .NET 6.0 | .NET 6.0 | 10.60 ms | 0.174 ms | 0.163 ms | 1.00 | 0.00 | | DecompressSharp | .NET 6.0 | .NET 6.0 | 16.59 ms | 0.262 ms | 0.232 ms | 1.57 | 0.03 | | | | | | | | | | | DecompressNative | .NET 7.0 | .NET 7.0 | 10.72 ms | 0.189 ms | 0.177 ms | 1.00 | 0.00 | | DecompressSharp | .NET 7.0 | .NET 7.0 | 14.27 ms | 0.245 ms | 0.229 ms | 1.33 | 0.02 | | | | | | | | | | | DecompressNative | .NET 8.0 | .NET 8.0 | 10.62 ms | 0.161 ms | 0.125 ms | 1.00 | 0.00 | | DecompressSharp | .NET 8.0 | .NET 8.0 | 10.99 ms | 0.113 ms | 0.106 ms | 1.04 | 0.02 | | | | | | | | | | | DecompressNative | .NET Core 3.1 | .NET Core 3.1 | 10.56 ms | 0.138 ms | 0.129 ms | 1.00 | 0.00 | | DecompressSharp | .NET Core 3.1 | .NET Core 3.1 | 16.26 ms | 0.134 ms | 0.119 ms | 1.54 | 0.02 | | | | | | | | | | | DecompressNative | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 10.56 ms | 0.102 ms | 0.095 ms | 1.00 | 0.00 | | DecompressSharp | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 17.07 ms | 0.086 ms | 0.077 ms | 1.62 | 0.02 |

I am noticing a slight improvement for .NET 7 and 8, and either no change or a slight regression for some earlier frameworks. What do you think?

oleg-st commented 2 weeks ago

I'll run benchmarks later too. I also need to check that a .NET Native project builds with ZstdSharp dependency, since there was a problem with that earlier. It may not support some modern c# stuff. See https://github.com/adamhathcock/sharpcompress/issues/793

oleg-st commented 2 weeks ago

My results:

Before

| Method | Job | Runtime | Mean | Error | StdDev | Ratio | |----------------- |--------------------- |--------------------- |----------:|----------:|----------:|------:| | CompressNative | .NET 6.0 | .NET 6.0 | 24.242 ms | 0.0543 ms | 0.0481 ms | 1.00 | | CompressSharp | .NET 6.0 | .NET 6.0 | 33.341 ms | 0.0609 ms | 0.0508 ms | 1.38 | | | | | | | | | | CompressNative | .NET 7.0 | .NET 7.0 | 24.065 ms | 0.0419 ms | 0.0392 ms | 1.00 | | CompressSharp | .NET 7.0 | .NET 7.0 | 32.441 ms | 0.0369 ms | 0.0288 ms | 1.35 | | | | | | | | | | CompressNative | .NET 8.0 | .NET 8.0 | 24.037 ms | 0.0394 ms | 0.0369 ms | 1.00 | | CompressSharp | .NET 8.0 | .NET 8.0 | 29.549 ms | 0.0431 ms | 0.0403 ms | 1.23 | | | | | | | | | | CompressNative | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 24.298 ms | 0.0394 ms | 0.0369 ms | 1.00 | | CompressSharp | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 37.369 ms | 0.0273 ms | 0.0213 ms | 1.54 | | | | | | | | | | DecompressNative | .NET 6.0 | .NET 6.0 | 5.676 ms | 0.0176 ms | 0.0165 ms | 1.00 | | DecompressSharp | .NET 6.0 | .NET 6.0 | 7.412 ms | 0.0174 ms | 0.0163 ms | 1.31 | | | | | | | | | | DecompressNative | .NET 7.0 | .NET 7.0 | 5.628 ms | 0.0120 ms | 0.0106 ms | 1.00 | | DecompressSharp | .NET 7.0 | .NET 7.0 | 6.868 ms | 0.0127 ms | 0.0099 ms | 1.22 | | | | | | | | | | DecompressNative | .NET 8.0 | .NET 8.0 | 5.624 ms | 0.0140 ms | 0.0130 ms | 1.00 | | DecompressSharp | .NET 8.0 | .NET 8.0 | 5.963 ms | 0.0108 ms | 0.0101 ms | 1.06 | | | | | | | | | | DecompressNative | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 5.748 ms | 0.0139 ms | 0.0130 ms | 1.00 | | DecompressSharp | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 8.349 ms | 0.0084 ms | 0.0065 ms | 1.45 |

After

| Method | Job | Runtime | Mean | Error | StdDev | Ratio | |----------------- |--------------------- |--------------------- |----------:|----------:|----------:|------:| | CompressNative | .NET 6.0 | .NET 6.0 | 24.251 ms | 0.0616 ms | 0.0576 ms | 1.00 | | CompressSharp | .NET 6.0 | .NET 6.0 | 33.360 ms | 0.0796 ms | 0.0745 ms | 1.38 | | | | | | | | | | CompressNative | .NET 7.0 | .NET 7.0 | 24.080 ms | 0.0526 ms | 0.0492 ms | 1.00 | | CompressSharp | .NET 7.0 | .NET 7.0 | 32.611 ms | 0.1105 ms | 0.0980 ms | 1.35 | | | | | | | | | | CompressNative | .NET 8.0 | .NET 8.0 | 24.074 ms | 0.0484 ms | 0.0429 ms | 1.00 | | CompressSharp | .NET 8.0 | .NET 8.0 | 29.681 ms | 0.0453 ms | 0.0423 ms | 1.23 | | | | | | | | | | CompressNative | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 24.298 ms | 0.0424 ms | 0.0396 ms | 1.00 | | CompressSharp | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 37.402 ms | 0.0396 ms | 0.0309 ms | 1.54 | | | | | | | | | | DecompressNative | .NET 6.0 | .NET 6.0 | 5.625 ms | 0.0160 ms | 0.0150 ms | 1.00 | | DecompressSharp | .NET 6.0 | .NET 6.0 | 7.417 ms | 0.0185 ms | 0.0173 ms | 1.32 | | | | | | | | | | DecompressNative | .NET 7.0 | .NET 7.0 | 5.639 ms | 0.0122 ms | 0.0114 ms | 1.00 | | DecompressSharp | .NET 7.0 | .NET 7.0 | 6.858 ms | 0.0085 ms | 0.0071 ms | 1.22 | | | | | | | | | | DecompressNative | .NET 8.0 | .NET 8.0 | 5.635 ms | 0.0131 ms | 0.0123 ms | 1.00 | | DecompressSharp | .NET 8.0 | .NET 8.0 | 5.969 ms | 0.0188 ms | 0.0176 ms | 1.06 | | | | | | | | | | DecompressNative | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 5.733 ms | 0.0113 ms | 0.0106 ms | 1.00 | | DecompressSharp | .NET Framework 4.6.2 | .NET Framework 4.6.2 | 8.352 ms | 0.0226 ms | 0.0212 ms | 1.46 |

Benchmarks LGTM. A .NET Native project builds with ZstdSharp dependency.

oleg-st commented 2 weeks ago

@teo-tsirpanis Thank you!