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

Remove Unsafe usage from GifDecoderCore and optimize loops #2851

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Dec 17, 2024

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Originally wanted to straightly PR the idea in #2850, but felt like we need to do something about the high amount of unsafe code in GifDecoderCore before touching anything there. By simply removing Unsafe, I noticed a minor perf drop. I managed compensate it by changing the inner loops so the JIT can remove bounds checks at least for one of the spans.

Benchmarks

BenchmarkDotNet v0.14.0, Windows 11 (10.0.22631.4602/23H2/2023Update/SunValley3)
12th Gen Intel Core i7-1280P, 1 CPU, 20 logical and 14 physical cores
.NET SDK 9.0.100
  [Host]     : .NET 8.0.11 (8.0.1124.51707), X64 RyuJIT AVX2
  Job-JOLLXK : .NET 8.0.11 (8.0.1124.51707), X64 RyuJIT AVX2

Runtime=.NET 8.0  Arguments=/p:DebugType=portable  IterationCount=3
LaunchCount=1  WarmupCount=3

//// main ////

| Method               | TestImage      | Mean       | Error      | StdDev    | Ratio | RatioSD | Gen0     | Gen1     | Gen2     | Allocated  | Alloc Ratio |
|--------------------- |--------------- |-----------:|-----------:|----------:|------:|--------:|---------:|---------:|---------:|-----------:|------------:|
| 'System.Drawing Gif' | Gif/cheers.gif |   4.503 ms |  0.8688 ms | 0.0476 ms |  1.00 |    0.01 | 328.1250 | 328.1250 | 328.1250 | 3011.75 KB |        1.00 |
| 'ImageSharp Gif'     | Gif/cheers.gif | 116.477 ms | 21.8437 ms | 1.1973 ms | 25.87 |    0.33 |        - |        - |        - |  135.04 KB |        0.04 |

//// Just dropping Unsafe ////
| Method               | TestImage      | Mean       | Error     | StdDev    | Ratio | RatioSD | Gen0     | Gen1     | Gen2     | Allocated  | Alloc Ratio |
|--------------------- |--------------- |-----------:|----------:|----------:|------:|--------:|---------:|---------:|---------:|-----------:|------------:|
| 'System.Drawing Gif' | Gif/cheers.gif |   4.536 ms |  2.058 ms | 0.1128 ms |  1.00 |    0.03 | 328.1250 | 328.1250 | 328.1250 | 3011.69 KB |        1.00 |
| 'ImageSharp Gif'     | Gif/cheers.gif | 120.442 ms | 25.420 ms | 1.3934 ms | 26.56 |    0.63 |        - |        - |        - |  135.04 KB |        0.04 |

//// PR ////
| Method               | TestImage      | Mean       | Error     | StdDev    | Ratio | RatioSD | Gen0     | Gen1     | Gen2     | Allocated  | Alloc Ratio |
|--------------------- |--------------- |-----------:|----------:|----------:|------:|--------:|---------:|---------:|---------:|-----------:|------------:|
| 'System.Drawing Gif' | Gif/cheers.gif |   4.456 ms |  1.580 ms | 0.0866 ms |  1.00 |    0.02 | 328.1250 | 328.1250 | 328.1250 | 3011.75 KB |        1.00 |
| 'ImageSharp Gif'     | Gif/cheers.gif | 116.112 ms | 28.261 ms | 1.5491 ms | 26.07 |    0.53 |        - |        - |        - |  135.04 KB |        0.04 |

Span<TPixel> row = imageFrame.PixelBuffer.DangerousGetRowSpan(writeY);

// Take the descriptorLeft..maxX slice of the row, so the loop can be simplified.
row = row[descriptorLeft..maxX];
Copy link
Member

Choose a reason for hiding this comment

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

This is so much neater. I'm sure there's other places we do bounds based looping in decoders like this.

@JimBobSquarePants JimBobSquarePants merged commit e08651b into main Dec 18, 2024
8 checks passed
@JimBobSquarePants JimBobSquarePants deleted the af/safe-GifDecoder branch December 18, 2024 01:49
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.

2 participants