Skip to content

Cleanup code and improve reliability. #38

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jul 11, 2024
Merged

Cleanup code and improve reliability. #38

merged 9 commits into from
Jul 11, 2024

Conversation

teo-tsirpanis
Copy link
Contributor

This PR:

  • Removes the dependency to the InlineIL package. All its uses can be replaced by the System.Runtime.CompilerServices.Unsafe package or other built-in .NET APIs, and the replaced code is simpler.
  • Updates the high-level Compressor and Decompressor classes to use classes derived from SafeHandle to manage the ZSTD context pointers. While safe handles are usually used for interop with native code, we use them here in managed code to manage the lifetime of manually allocated memory. This makes the code simpler and much more reliable; you don't have to write GC.KeepAlives anymore, and safe handles protect from cases like one thread calling Dispose while another thread uses the resource.
  • Removes two cases where spans were compared to null. This is not valid code since spans are structs and not classes.

@teo-tsirpanis
Stop using InlineIL.
fb154f5 
It can be entirely replaced by built-in APIs that are available on all targeted frameworks.
@teo-tsirpanis
Add SafeHandles for the compression and decompression contexts.
b4115b6
@teo-tsirpanis
Use safe handles in Compressor and Decompressor.
6988459 
The classes' implementation became simpler, safer, and less error-prone.
@teo-tsirpanis
Remove invalid comparisons of spans to null.
ea68b1c
teo-tsirpanis
teo-tsirpanis commented Jul 7, 2024
View reviewed changes
Copy link
Contributor Author

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

Left some additional comments.

oleg-st
oleg-st reviewed Jul 8, 2024
View reviewed changes
@oleg-st
Copy link
Owner

oleg-st commented Jul 8, 2024 •
edited
Loading

  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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
oleg-st reviewed Jul 9, 2024
View reviewed changes
@oleg-st
Copy link
Owner

oleg-st commented Jul 9, 2024

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 adamhathcock/sharpcompress#793

@oleg-st
Copy link
Owner

oleg-st commented Jul 9, 2024 •
edited
Loading

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 oleg-st merged commit d3032cb into oleg-st:master Jul 11, 2024
@teo-tsirpanis teo-tsirpanis deleted the cleanup branch July 11, 2024 20:53
@oleg-st
Copy link
Owner

oleg-st commented Jul 11, 2024

@teo-tsirpanis Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-st oleg-st oleg-st left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@teo-tsirpanis @oleg-st