Text segment ReadOnlySpan<byte> initialization #1133

Sergio0694 · 2020-02-27T19:31:37Z

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 matches 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

This PR includes a couple of minor optimizations:

Replaced some static readonly byte[] arrays with static ReadOnlySpan<byte> values that directly map over constant data in the .text segment (see here).
Removed some bound checks when accessing those static sections (added some DEBUG checks)
Removed some type initialization calls to Encoding.ASCII.GetBytes, no longer necessary
Switched the Stream.Write calls to the ReadOnlySpan<byte> overloads.

No rush at all for this, just saw those byte[] arrays and thought I'd have some fun optimizing them away. Pinging @antonfirsov for the reviews as he's the official performance guru. 😄

antonfirsov

Let's prove that the new unsafe code does not open a security hole! Otherwise looks good.

src/ImageSharp/Formats/Jpeg/Components/ZigZag.cs

antonfirsov · 2020-02-27T23:02:32Z

src/ImageSharp/Formats/Png/Zlib/DeflaterHuffman.cs

+            DebugGuard.MustBeLessThan(toReverse & 0xF, Bit4Reverse.Length, nameof(toReverse));
+            DebugGuard.MustBeLessThan((toReverse >> 4) & 0xF, Bit4Reverse.Length, nameof(toReverse));
+            DebugGuard.MustBeLessThan((toReverse >> 8) & 0xF, Bit4Reverse.Length, nameof(toReverse));
+            DebugGuard.MustBeLessThan(toReverse >> 12, Bit4Reverse.Length, nameof(toReverse));
+
+            ref byte bit4ReverseRef = ref MemoryMarshal.GetReference(Bit4Reverse);
+
+            return (short)(Unsafe.Add(ref bit4ReverseRef, toReverse & 0xF) << 12
+                           | Unsafe.Add(ref bit4ReverseRef, (toReverse >> 4) & 0xF) << 8
+                           | Unsafe.Add(ref bit4ReverseRef, (toReverse >> 8) & 0xF) << 4
+                           | Unsafe.Add(ref bit4ReverseRef, toReverse >> 12));


DebugGuard won't save us in production. Where does toReverse come from? Can a malicious input result in buffer overflow?

Sure, I mostly added those to validate the code path in our CI tests. I had not considered possible attempts to exploit this from a security standpoint, you're right 😅

What if we figured out the upper/lower bounds of those 4 indices, and validated just those? It would still result in just 2 conditional jumps, instead of 4 (one for each access).

Actually, now that I think about it:

The first three indices are always computed with a final & 0xF, which means the maximum possible value is 0xF, so 15, which is always in range. So those first 3 accesses should always be necessarily valid, without the need to test them

That last >> 12 could result in an out of bound index, either if the value is negative and below -1 << 12, or if it's a positive value greater than 1 << 12. I guess we could just check this last one just to be extra sure, as I don't know where exactly the input is coming from here, as you mentioned.

That's still just a single branch compared to 4, so... Yay? 😁
What do you think?

Sounds good, let's do it like that!

Awesome, done in 0577690. Also added a detailed comment for future reference 😊

My comment on BitLengthOrder also applies here. Additionally, if we already on the microoptimization road, we should cache the value of toReverse >> 12 to a local, since we are using it twice.

Done in 4b0dfd1.

Also I should really stress out how if we care about micro-optimizations, we should really consider refactoring our Guard APIs with that trick I suggested. Look at all that asm clutter due to that string.Format call we have here:

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8B6YgAgGEIAHATygEsBzACwzIAowBKMgZQYIyAGWzBouMtgB2AEzKRpGRsACuGCQDoAsAChSIhmBjTcMearkwoZDCxhkAgtWxh7h46ZhoyANWu4DBDSZABMmgAMOrp6AAKhAIxxEWSxCZoAIgzYTNIQuBhGuADcyanpWTl5BUWaVLIwjtLYADa0gSVlaZoASpaF+DB1EPjUDC3WfNYAbkYwnbqxKd19SgyDmgCSStY0U1CzxgtxAMypoWQZMABmLdgY1gASqtfX+DJ6AN56ZL9k1IxpvcHGkkGQejBsLIAPLSNp8FzSAA8wFoDwAfGQAEIMDAoCHTAIOAC8mOkMAA7mRUQ8ANoAXTInzIER8AA4fOgyAlQj5edzWWQkD4ElyEj4AJw+ACsIpOPnl3PFZAA7CLpWQAL6lXR/Mg/P6kchI3CqfDvKC0dEG35G8EwQlQMy2dzAXGSCDXKTcsFu9hAlqqIY2sh2pHEU3m7CW626w0GJEuKDYfBkZqDYkAIg0BKJmfRvlaQdsEDIsEdzr9uHDSZTsb1YdiKtwBaLDgpuJY1PdZYdRNk4abLZDtIAsjA7BBZJtRi0OOPJ9PZ9DqIVgrhNI4mExYLhAoTti0GNJj0xuPSQ7EzqCyLgWNB2DiMLmnTAOMf2Dm+6/uCHvnG9V+D8SxfMwemYNg+BYBhrgwLFaB5MhiRA79nXRTEeR1QC/gAcVUaNZE0UdVAKLEYGEeZcAAFRYGRoSgABRABHfCWioiAkVUD90Q4DguKUbgvwrGBwNYDAoJguCEP5BJZTTFMYE9DghKJbhuB1EM9VgL0aQcP18VQkSbiQ3svXHfBoFoUdozvVpNBwicIWuawTGMDgnwM4T1L0TS/ibTg7wfbgOAAVVMbBnM3WRZA4bTuzxUCjOuHwVNfMgADIWQQAAxXgkSRblQl87CSr1QgyDC3AIqGRxoti4z9MSpyfGUiBErIdCyBQXhMoiHK8oKtlitK0rysq6qopiuLGsM5rOFStDMTZHqstysh8q64aRuwsbwsi2qpoa3FPKJOaFpEiDxOg2D4J5NSsL+TU9CemJ9ATSMLStPQ7QABSgCBZgaSRBkXSQNH+f6HjAdgcmwY8CjIY8AwYeRaxBgJojDCMzU+2NaSuNRt0mB5qBo/7VFYC9dFpBiEDAQMGmy/78HqGAqEdHIYCpj9rGaFpyjBJNClac4yDwgivhDMMPujL6ANDAx/EYa4GHmF17hdBxcGoGAwBg1X5ADYsGEkCY93VkJoDIGAWJF8HsDId4EHWM0yCNmApYMGR5Dsf6KUkGRrbpmBVyCEIYMR9gTbTCAMGiBsE2xqMY09400fkjNM3d/MaIcDBoyYCc3bbHwKWgtxb3vVQWnkYAHGR2RgUImtozrVP1vT9MYCzJ2c/cJ2XdTd3NBb5N8HreM09b1Mu6zNGJ2sAA5BS+4cLuyE9TX/mnhebDsDXo/BuuFHsMAAGtzBH4g0Yn20EwwWgdc7hSsyowtAxgVfbEfhxN7seu2xXwfk/aet8FbGhgMHUOwQFDaSzI4KATAzQmAwLTYw0DpD5nbomaecVZ5ZzbJmYgmJo47khA8PetEQj/0dtgZ2+BXbD2wcQSB6C1zSDAWOCc94lzUDnAuHhM4+ErnYRuLcZC9wMAPHCY8p5zyXmvAkMEsQUBkGIqRcilEaJ0UYrbNiHE35th4oYj+xcP4+BMcWJ2Pg0gpHnhQ5egxfzyz1GXFyZBLEOBAGQTYVBRit2ABMJEniwH/lKhHDgw8/FJhgOxDgTteCYgiM40qYTtoeJYH7BBSDBhKGhOoaE1wegyELmgkO7COD2KXgpHwAASTM79iyfHdpqR2JF2DHzNmDKhG8bA21YiWJkTtNSaEzN5Fxj0QwvRHAIqcQj+HcLmcuDBG5F4QEPLI6QZ4qZ6gBFI4EAtUiqLJhACk2TkF5IKUUkpMAykYI4LY7eY9d6OO8OUFIgw9yc14KSYavtTlpkpE4RBFyMD5LBdcrZtyoEVKqVAV5PhPlVULuMl6QA.

Well this is just terrible, wish we have noticed that earlier. Guard.MustBeLessThanOrEqualTo will make this method actually slower than it was before your changes. Can you just use a manual check + ThrowHelper here?

If you have some time, feel free to file a PR in SixLabors/SharedInfrastructure. For the comparison methods I'd rather see a set of duplicate non-generic methods, than TValue : IComparable<TValue>, which also brings some extra overhead I believe.

src/ImageSharp/Processing/Processors/Quantization/OctreeFrameQuantizer{TPixel}.cs

JimBobSquarePants · 2020-02-28T04:28:40Z

src/ImageSharp/Formats/Png/Zlib/DeflaterHuffman.cs

@@ -40,8 +40,6 @@ internal sealed unsafe class DeflaterHuffman : IDisposable
        // probability, to avoid transmitting the lengths for unused bit length codes.
        private static readonly int[] BitLengthOrder = { 16, 17, 18, 0, 8, 7, 9, 6, 10, 5, 11, 4, 12, 3, 13, 2, 14, 1, 15 };


We might well be better off using the ReadOnlySpan<byte> here also and casting.

I know we do that in the jpeg encoder for uint and BitCountLut with, as I recall, a net gain in performance.

Done in 5b7ac75.
Should I add some bounds check here, or are all those accesses guaranteed to be safe?

I couldn't tell you for certain I'm afraid. That port was an act of desperation over understanding.

I see. Should I just switch those accesses to a direct Span<T>[int] access then? 🤔

If we lack understanding, we shall go for safety.

Btw if we use the data in Bit4Reverse for indexing, elements shouldn't be byte-s. It's more work for the CPU:
https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQcAMACbckB0ASgK4B2wAllAKYEDCA9lAA6UA2NATgMrcBulAMY0IAbnTo4AZjwJs9bAG90uXKrUsulfmGA08SAGzYARgE992AJJkAJjQAet4NIQAKLjQBmZywZYAGmxKChD7JwBKDVwVNDUEvAB2bABVMggwbzoAQTs7Tx9sIPCHR0iJeLUAXxjsOq0dPQN8EwsrWzKAIX9C33aA4IHSqLq4xLU4FPTM7II8gq9fEtCyirratGqgA==

I get what you're saying now, yeah 👍
I'm not sure I understand where you're seeing that with Bit4Reverse though.
Here's where that is used:

ImageSharp/src/ImageSharp/Formats/Png/Zlib/DeflaterHuffman.cs

Lines 384 to 392 in 4b0dfd1

int toReverseRightShiftBy12 = toReverse >> 12;

Guard.MustBeLessThanOrEqualTo<uint>((uint)toReverseRightShiftBy12, 15, nameof(toReverse));

ref byte bit4ReverseRef = ref MemoryMarshal.GetReference(Bit4Reverse);

return (short)(Unsafe.Add(ref bit4ReverseRef, toReverse & 0xF) << 12

| Unsafe.Add(ref bit4ReverseRef, (toReverse >> 4) & 0xF) << 8

| Unsafe.Add(ref bit4ReverseRef, (toReverse >> 8) & 0xF) << 4

| Unsafe.Add(ref bit4ReverseRef, toReverseRightShiftBy12));

All the indexing values there are int already 🤔
I mean, those are actually the same indices that were being used before as well. I'm not sure I'm following here, where is the byte offsetting peformed?

Ahh sorry, I was wrong the values for Bit4Reverse are not used for indexing into data. This is the data that gets indexed.

We should deal with Guard however to make sure we don't regress. Do you plan to fix it globally in SixLabors/SharedInfrastructure, or will you replace it in this PR with a manual check + ThrowHelper calls?

Ahahah no problem, glad we could figure this out, I was wondering what was I missing there 😄

As for Guard, I'd say that's an unrelated improvement, since it's not something that will impact the functionality in any way, it's just a matter of further optimizations. I guess the best thing would be to just make another PR and fix the entire Guard class, with all the included APIs. Sounds good?

If you can do it, that would be the best!

Sure, always happy to bring more of my discoveries and optimizations to ImageSharp! 😄

JimBobSquarePants

Got failing tests here.

https://github.com/SixLabors/ImageSharp/pull/1133/checks?check_run_id=473799814#step:11:195

Sergio0694 · 2020-02-28T11:55:47Z

@JimBobSquarePants Whoops, fixed in 68387a7, my bad 😅

codecov · 2020-02-28T12:11:25Z

Codecov Report

Merging #1133 into master will decrease coverage by <.01%.
The diff coverage is 98.63%.

@@            Coverage Diff             @@
##           master    #1133      +/-   ##
==========================================
- Coverage   82.24%   82.23%   -0.01%     
==========================================
  Files         678      678              
  Lines       29164    29192      +28     
  Branches     3284     3284              
==========================================
+ Hits        23986    24007      +21     
- Misses       4481     4488       +7     
  Partials      697      697

Flag	Coverage Δ
#unittests	`82.23% <98.63%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
...ats/PixelBlenders/PorterDuffFunctions.Generated.cs	`11.73% <ø> (ø)`	⬆️
...c/ImageSharp/Common/Extensions/StreamExtensions.cs	`93.33% <ø> (ø)`	⬆️
src/ImageSharp/Formats/Png/PngConstants.cs	`100% <100%> (ø)`	⬆️
...rc/ImageSharp/Metadata/Profiles/Exif/ExifWriter.cs	`80% <100%> (ø)`	⬆️
src/ImageSharp/Formats/Png/PngEncoderCore.cs	`97.63% <100%> (ø)`	⬆️
src/ImageSharp/Formats/Jpeg/Components/ZigZag.cs	`100% <100%> (ø)`	⬆️
...ssors/Quantization/OctreeFrameQuantizer{TPixel}.cs	`95.56% <100%> (+0.02%)`	⬆️
...Sections/GifNetscapeLoopingApplicationExtension.cs	`100% <100%> (ø)`	⬆️
src/ImageSharp/Formats/Gif/GifEncoderCore.cs	`94.28% <100%> (ø)`	⬆️
src/ImageSharp/Formats/Gif/GifConstants.cs	`100% <100%> (ø)`	⬆️
... and 3 more

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 a9e10e2...20bf5fa. Read the comment docs.

JimBobSquarePants · 2020-03-03T13:55:05Z

@Sergio0694 @antonfirsov Are we waiting for this to be completed and merged before finishing here?

SixLabors/SharedInfrastructure#7

Sergio0694 · 2020-03-03T15:23:56Z

@JimBobSquarePants Not sure whether we necessarily have to wait for that. I mean, having that would reduce the overhead in those checks, but it wouldn't actually change anything semantically. We might want to wait for it though in case the changes in this PR alone have too much of a performance regression, but that shouldn't be the case (plus it'd be temporary anyways).

Up to you guys 👍

antonfirsov · 2020-03-03T22:38:04Z

@JimBobSquarePants I expect the aggregate result of the changes in DeflaterHuffman.BitReverse() to be a regression, unless we finish SixLabors/SharedInfrastructure#7 or replace Guard with verbose local code.

On the other hand, I'm fine fixing the regression in a follow-up PR.

JimBobSquarePants · 2020-03-04T12:54:06Z

@Sergio0694 I found the AccessViolation.

https://github.com/SixLabors/ImageSharp/pull/1133/checks?check_run_id=484873388#step:11:698

It's not us it's the ImageMagick reference codec. @dlemstra You'll want to see this.

Sergio0694 · 2020-03-04T13:23:35Z

@JimBobSquarePants That's awesome, great work! Props to your investigative skills 😄
Also glad to hear it's not actually coming from ImageSharp, that's perfect.

JimBobSquarePants · 2020-03-06T06:10:53Z

Ok.... The submodule is now updated and I've added updated relevant tests.

~~I've also configured the solution to build the T4 templates on build since we don't import the transformed file.~~

Will merge once this finishes building.

Spoke too soon... The templates don't automatically run on Unix of course because there's no Visual Studio. Will have another think.

Update:
I've added a build target to copy the generated file from the submodule to the main solution to ensure we capture it.

dlemstra · 2020-03-06T10:58:07Z

tests/ImageSharp.Tests/Image/ImageFrameCollectionTests.NonGeneric.cs

@@ -90,7 +90,7 @@ public void AddNewFrame_Frame_FramesNotBeNull()
                        this.Collection.AddFrame(null);
                    });

-                Assert.StartsWith("Value cannot be null.", ex.Message);
+                Assert.StartsWith("Parameter \"source\" must be not null.", ex.Message);


Could we also do Assert.Throws<ArgumentNullException>("source", () => here instead?

I thought that was dealt with above?

Sergio0694 added 8 commits February 27, 2020 20:01

Refactored byte[] array in ZigZag type

4e8de95

Refactored byte[] array in DeflaterHuffman type

c83d066

Refactored byte[] array in PngConstants type

aa65526

Refactored byte[] array in ExifConstants type

da79756

Refactored byte[] array in Octree type

4dbec4c

Refactored byte[] arrays in GifConstants type

714c960

Refactored byte[] arrays in ProfileResolver type

bb7b041

Code style tweaks

f907f90

Sergio0694 requested a review from antonfirsov February 27, 2020 19:31

Sergio0694 self-assigned this Feb 27, 2020

Sergio0694 added API area:performance labels Feb 27, 2020

Sergio0694 added this to the 1.0.0 milestone Feb 27, 2020

Fixed a build error

8ad8a59

antonfirsov requested changes Feb 27, 2020

View reviewed changes

Sergio0694 added 2 commits February 28, 2020 01:03

Minor code changes to improve clarity

b251c00

Added input validation to DeflaterHuffman unsafe offsetting

0577690

JimBobSquarePants reviewed Feb 28, 2020

View reviewed changes

JimBobSquarePants requested changes Feb 28, 2020

View reviewed changes

Fixed a test in the DeflaterHuffman type

68387a7

Sergio0694 added 2 commits February 28, 2020 12:56

Merge branch 'master' into sp/text-segment-initialization

f3f6d9c

Refactored DeflaterHuffman.BitLengthOrder to ReadOnlySpan<byte>

5b7ac75

Sergio0694 and others added 3 commits February 28, 2020 16:25

Reintroduced some bounds checks for additional security

70ed21e

Minor micro-optimization in DeflaterHuffman type

4b0dfd1

Merge branch 'master' into sp/text-segment-initialization

719b5a3

Merge branch 'master' into sp/text-segment-initialization

99b88c0

JimBobSquarePants added 2 commits March 6, 2020 15:55

tem remove submodule to reset dirty

b43a66c

Fix module, import configs, automate T4 builds

cbd1872

Manually include compiled file in source via auto copy.

20bf5fa

JimBobSquarePants approved these changes Mar 6, 2020

View reviewed changes

JimBobSquarePants merged commit a49ebb0 into master Mar 6, 2020

JimBobSquarePants deleted the sp/text-segment-initialization branch March 6, 2020 10:47

dlemstra reviewed Mar 6, 2020

View reviewed changes

JimBobSquarePants modified the milestones: 1.0.0, 1.0.0-rc1 Apr 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Text segment ReadOnlySpan<byte> initialization #1133

Text segment ReadOnlySpan<byte> initialization #1133

Sergio0694 commented Feb 27, 2020

antonfirsov left a comment

antonfirsov Feb 27, 2020

Sergio0694 Feb 28, 2020 •

edited

Loading

antonfirsov Feb 28, 2020

Sergio0694 Feb 28, 2020

antonfirsov Feb 28, 2020

Sergio0694 Feb 28, 2020

antonfirsov Feb 28, 2020

JimBobSquarePants Feb 28, 2020

Sergio0694 Feb 28, 2020 •

edited

Loading

JimBobSquarePants Feb 28, 2020

Sergio0694 Feb 28, 2020

antonfirsov Feb 28, 2020

Sergio0694 Feb 28, 2020

antonfirsov Feb 28, 2020

Sergio0694 Feb 28, 2020

antonfirsov Feb 28, 2020

Sergio0694 Feb 28, 2020

JimBobSquarePants left a comment

Sergio0694 commented Feb 28, 2020

codecov bot commented Feb 28, 2020 •

edited

Loading

JimBobSquarePants commented Mar 3, 2020

Sergio0694 commented Mar 3, 2020

antonfirsov commented Mar 3, 2020

JimBobSquarePants commented Mar 4, 2020

Sergio0694 commented Mar 4, 2020

JimBobSquarePants commented Mar 6, 2020 •

edited

Loading

dlemstra Mar 6, 2020

JimBobSquarePants Mar 6, 2020

		@@ -40,8 +40,6 @@ internal sealed unsafe class DeflaterHuffman : IDisposable
		// probability, to avoid transmitting the lengths for unused bit length codes.
		private static readonly int[] BitLengthOrder = { 16, 17, 18, 0, 8, 7, 9, 6, 10, 5, 11, 4, 12, 3, 13, 2, 14, 1, 15 };

	int toReverseRightShiftBy12 = toReverse >> 12;
	Guard.MustBeLessThanOrEqualTo<uint>((uint)toReverseRightShiftBy12, 15, nameof(toReverse));

	ref byte bit4ReverseRef = ref MemoryMarshal.GetReference(Bit4Reverse);

	return (short)(Unsafe.Add(ref bit4ReverseRef, toReverse & 0xF) << 12
	\| Unsafe.Add(ref bit4ReverseRef, (toReverse >> 4) & 0xF) << 8
	\| Unsafe.Add(ref bit4ReverseRef, (toReverse >> 8) & 0xF) << 4
	\| Unsafe.Add(ref bit4ReverseRef, toReverseRightShiftBy12));

Text segment ReadOnlySpan<byte> initialization #1133

Text segment ReadOnlySpan<byte> initialization #1133

Conversation

Sergio0694 commented Feb 27, 2020

Prerequisites

Description

antonfirsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Sergio0694 commented Feb 28, 2020

codecov bot commented Feb 28, 2020 • edited Loading

Codecov Report

JimBobSquarePants commented Mar 3, 2020

Sergio0694 commented Mar 3, 2020

antonfirsov commented Mar 3, 2020

JimBobSquarePants commented Mar 4, 2020

Sergio0694 commented Mar 4, 2020

JimBobSquarePants commented Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 Feb 28, 2020 •

edited

Loading

Sergio0694 Feb 28, 2020 •

edited

Loading

codecov bot commented Feb 28, 2020 •

edited

Loading

JimBobSquarePants commented Mar 6, 2020 •

edited

Loading