Remove obsolete code #2189

JimBobSquarePants · 2022-07-25T07:36:07Z

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

Removes all obsolete conditions, obsolete methods, and fixes all compiler warnings.

gfoidl

Changes per-se LGTM, but I expect there will be some perf-regression. See comment about Span.Slice vs Range operator.
Maybe you can write a comment on the Roslyn-issue to help them prioritize that one 😉

gfoidl · 2022-07-25T12:32:11Z

src/ImageSharp/ColorSpaces/Companding/SRgbCompanding.cs

                {
                    // Vector4 fits neatly in pairs. Any overlap has to be equal to 1.
-                    Expand(ref MemoryMarshal.GetReference(vectors.Slice(vectors.Length - 1)));
+                    Expand(ref MemoryMarshal.GetReference(vectors[^1..]));


The old version may be more performant, unfortunately. This is tracked in dotnet/roslyn#43598, and you can see the difference in this sharplab.

~~If it's not perf-sensitive, then got with the new code, as it's cleaner to read.~~
Edit: this goes through that whole PR, there are definitely some hot places where this will have negative impact.
If Roslyn tackles that issue, perf will increase "for free".

🤦‍♂️ This is so frustrating. Why introduce an analyser that promotes worse behaviour?

gfoidl · 2022-07-25T12:38:20Z

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

-                a = b;
-                b = tmp;
-            }
+            static void Swap(ref short a, ref short b) => (b, a) = (a, b);


Just as info:
Resulting machine code will be the same, but the IL-size is with the tuple-based approach a bit larger than with the local (old version).

I don't think it matters here, but potential drawbacks

JIT may give up inlining if any budget may exceed earlier --> add MethodImpl.AggressiveInlining (or the ImageSharp-equivalent) to be on the safe side?

on products where IL size matters this could have a negative effect (TBH I don't know if two bytes more or less count here, as the reduction overall is much bigger that this PR does, but just to point it out)

Another poor analyser suggestion 😔

I'm no compiler engineer*, but for me this pattern is easy to recognize, so "optimal" code should be emitted, even if Roslyn isn't a optimizing compiler (that's the role of the JIT), but regarding IL-size optimizations count towards Roslyn.

* so I have somewhat limited understanding on how things work there

gfoidl · 2022-07-25T12:42:40Z

src/ImageSharp/Formats/Png/PngDecoderCore.cs


            int zeroIndex = data.IndexOf((byte)0);
-            if (zeroIndex < PngConstants.MinTextKeywordLength || zeroIndex > PngConstants.MaxTextKeywordLength)
+            if (zeroIndex is < PngConstants.MinTextKeywordLength or > PngConstants.MaxTextKeywordLength)


Just FYI: dotnet/roslyn#60534

I'd leave the code as is, as it's basically the same as now just nicer to read. When Roslyn got that addressed the perf-goodness comes "for free".

MO ANALYZERS MO PROBLEMS

gfoidl · 2022-07-25T12:46:40Z

src/ImageSharp/Formats/Webp/Lossy/QuantEnc.cs

-                    Vp8ModeScore tmp = rdCur;
-                    rdCur = rdBest;
-                    rdBest = tmp;
+                    (rdBest, rdCur) = (rdCur, rdBest);


Use the swap-method from above (where I commented) instead (needs to be made generic then)?
Or drop the swap-method and write the one-line directly?

I don't have a strong opinion what's better here, but it should be consistent.
Leaning towards swap-method, as if there's a super nice trick once on how to swap more efficiently only one place needs to be updated.

👍 good idea

src/ImageSharp/Formats/Webp/Lossy/Vp8EncIterator.cs

src/ImageSharp/Common/Helpers/RuntimeUtility.cs

antonfirsov

Looks good, but somewhat concerned about var rules.

antonfirsov · 2022-09-02T13:04:47Z

src/ImageSharp/Common/Helpers/SimdUtils.ExtendedIntrinsics.cs


-                    Vector<ushort> u0 = Vector.Narrow(w0, w1);
-                    Vector<ushort> u1 = Vector.Narrow(w2, w3);
+                    var u0 = Vector.Narrow(w0, w1);


In case of arrays the PR replaces var with the more specific T[] on the left side, which feels like an inconsistent configuration of rules, and personally I always prefer to have the type information around in case of primitive types. It's not immediately obvious what does Narrow return here.

I’ve really struggled to get editorconfig to do what I want here (var rules are actually unchanged). This seems to fall under the “type is apparent” rule. There’s actually been a warning sitting in the code there for years, it just was ignored.

Why arrays aren’t considered apparent I simply do not know.

I don't think it's "apparent" when a non-generic method on a non-generic type returns a variable with a generic type of the same name. The lack of the type argument hurts readability, especially with API-s like Vector.Narrow. Is there a way to fine-tune this rule so it doesn't kick in for these cases? What if we disable it entirely?

I'm gonna create an empty project and see if I can get the rules working using the VS editorconfig editor and a fresh file.

Here's var turned off everywhere. It's verbose as hell but at least it's consistent.

Here's with "when type is apparent"

This one is annoying because the byte[] declaration should be apparent and the Vector.Narrow(...) return value isn't

Here's "when type is apparent" and "built in types"

This one is annoying because the double and int primitives are not apparent in my opinion since they require the reader to know the syntax declaration rules on the right hand side.

Which do you prefer?

I may be very oldscool, but for me the first one is the lesser evil. I wonder what do others think @SixLabors/core?

This rule/analyzer (IDE0007?) really sucks. I wonder if there is a better way to raise awareness about it than posting on https://developercommunity.visualstudio.com/ which is a straight way to oblivion.

I agree and with the forcing of target type new we handle known types. I'll implement that.

There's actually 2 issues in Roslyn around this. (One closed as a duplicate)

dotnet/roslyn#23714
dotnet/roslyn#55722

I wish I could do this though. It's really annoying that I can't seem to find a way.

byte[] b = [10];

antonfirsov · 2022-09-02T14:59:23Z

src/ImageSharp/PixelFormats/PixelBlenders/PorterDuffFunctions.Generated.tt

 // <auto-generated />

-<#
-// Note use of MethodImplOptions.NoInlining. We have tests that are failing on certain architectures when


So this was specific to old runtimes?

Yep. All fixed now 😀

dlemstra · 2022-09-03T07:26:44Z

src/Directory.Build.props

  </PropertyGroup>

+  <PropertyGroup>
+    <CodeAnalysisRuleSet>..\ImageSharp.ruleset</CodeAnalysisRuleSet>


I tend to only use forward slashes now because Windows now also supports that.

src/ImageSharp/Formats/Png/PngHeader.cs

dlemstra · 2022-09-03T08:23:52Z

src/ImageSharp/Formats/Webp/Lossy/Vp8Encoding.cs

-            VerticalPred(dst.Slice(I16VE16), top, 16);
-            HorizontalPred(dst.Slice(I16HE16), left, 16);
-            TrueMotion(dst.Slice(I16TM16), left, top, 16);
+            DcMode(dst[..], left, top, 16, 16, 5);


I16DC16 seems to be missing here?

Turns out that I16DC16 is always zero. And that is probably why it was removed?

I'll re read the change.

No need for a slice at all so just pass dst

src/ImageSharp/PixelFormats/PixelImplementations/PixelOperations/Rgb24.PixelOperations.cs

dlemstra

Made a couple extra commits and I think we are good to go now for a merge.

dlemstra · 2022-09-06T16:28:20Z

src/ImageSharp/Color/Color.cs

-            Configuration configuration,
-            ReadOnlySpan<Color> source,
-            Span<TPixel> destination)
+        public static void ToPixel<TPixel>(ReadOnlySpan<Color> source, Span<TPixel> destination)


This is a breaking change in the Api? Should we not limit this PR to only remove non breaking changes?

I added Configuration to the parameter list to enable the optimization of this method by utilizing the PixelOperations<TPixel>.FromRgba64() batch conversion method. The abstract PixelOperations<TPixel> methods take Configuration for the case the implementation decides to refer to Configuration.MemoryAllocator to allocate temporary buffers.

If we don't like / don't need this pattern, we can open an issue to re-evaluate the design, after carefully analyzing our current conversion methods, which no longer need special logic for .NET Framework, though we may need to account for non-Intel, non-coreclr targets.

But I would avoid making sneaky public API changes in a big refactor PR. Is this the only one ore is there more?

Let's re-add it for now then and add a TODO in the code so intent is known.

Is this the only one ore is there more?

Some of the methods on ColorSpaceConverter are now static. I want to keep them like that though.

src/ImageSharp/ColorSpaces/Conversion/Implementation/Converters/HunterLabToCieXyzConverter.cs

src/ImageSharp/Formats/Jpeg/Components/Decoder/ArithmeticScanDecoder.cs

dlemstra · 2022-09-10T17:40:52Z

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

            remaining -= JFifMarker.Length;

-            JFifMarker.TryParse(this.temp, out this.jFif);
+            _ = JFifMarker.TryParse(this.temp, out this.jFif);


Should we not handle this?

Yeah, you're right. Now explicitly throwing NotSupportedException since we only support JFIF App0 markers.

src/ImageSharp/Formats/Png/PngDecoderCore.cs

src/ImageSharp/Formats/Webp/Lossy/WebpLossyDecoder.cs

src/ImageSharp/Metadata/Profiles/Exif/ExifReader.cs

src/ImageSharp/Metadata/Profiles/ICC/MultiProcessElements/IccMatrixProcessElement.cs

src/ImageSharp/Metadata/Profiles/ICC/TagDataEntries/IccXyzTagDataEntry.cs

JimBobSquarePants · 2022-09-12T01:18:33Z

I'm gonna merge this now. All tests are passing, and I've covered any raised issues.

JimBobSquarePants added 3 commits July 24, 2022 00:25

Remove SUPPORTS conditions

eee64f2

Remove other obsolete conditions and warnings

6fc84a8

Remove obsolete OSX transform methods

ca0b284

JimBobSquarePants added the codequality label Jul 25, 2022

JimBobSquarePants requested a review from a team July 25, 2022 07:36

JimBobSquarePants changed the title ~~Js/remove obsolete code~~ Remove obsolete code Jul 25, 2022

JimBobSquarePants added this to the 3.0.0 milestone Jul 25, 2022

gfoidl reviewed Jul 25, 2022

View reviewed changes

JimBobSquarePants added 4 commits July 26, 2022 17:33

Normalize some reference swapping

3fb4787

Clean up pixel blender defaults

3c40500

Update PixelBlender{TPixel}.cs

3d837b1

Merge branch 'main' into js/remove-obsolete-code

606e64a

dlemstra reviewed Sep 2, 2022

View reviewed changes

src/ImageSharp/Formats/Webp/Lossy/Vp8EncIterator.cs Show resolved Hide resolved

dlemstra reviewed Sep 2, 2022

View reviewed changes

src/ImageSharp/Common/Helpers/RuntimeUtility.cs Outdated Show resolved Hide resolved

JimBobSquarePants added 3 commits September 2, 2022 19:27

Fix build

89faddf

PR feedback

61299f9

Merge branch 'main' into js/remove-obsolete-code

65a2ea3

antonfirsov reviewed Sep 2, 2022

View reviewed changes

dlemstra reviewed Sep 3, 2022

View reviewed changes

Use BitOperations directly.

ba1adc3

dlemstra reviewed Sep 3, 2022

View reviewed changes

src/ImageSharp/Formats/Png/PngHeader.cs Show resolved Hide resolved

Did not push all files....

3031400

dlemstra reviewed Sep 3, 2022

View reviewed changes

src/ImageSharp/PixelFormats/PixelImplementations/PixelOperations/Rgb24.PixelOperations.cs Show resolved Hide resolved

dlemstra added 3 commits September 3, 2022 10:54

Removed unused argument.

f9c9a01

Removed unit tests.

1c5a78c

Removed unused class.

3a6f26d

dlemstra reviewed Sep 3, 2022

View reviewed changes

JimBobSquarePants added 2 commits September 6, 2022 23:29

Update editorconfig and fix all new warnings.

b22785e

Fix CA1854 for NET 7

ac84e49

JimBobSquarePants added 2 commits September 6, 2022 23:54

Cleanup and remove unused index

90c85c0

Use culture for NET 7

0e46510

dlemstra reviewed Sep 6, 2022

View reviewed changes

src/ImageSharp/ColorSpaces/Conversion/Implementation/Converters/HunterLabToCieXyzConverter.cs Outdated Show resolved Hide resolved

JimBobSquarePants added 2 commits September 7, 2022 07:51

Reverse unsealing

e25734a

Reverse breaking change.

9b29319

JimBobSquarePants mentioned this pull request Sep 7, 2022

Implement Median Blur processor #2219

Merged

4 tasks

Merge branch 'main' into js/remove-obsolete-code

145dc9e

JimBobSquarePants requested review from antonfirsov and dlemstra September 9, 2022 22:59