Repository navigation
Some unit tests fail with SSE disabled (i.e. scalar path only) #2414
Description
Activity
It looks like CI runs the tests on the agents "as is", so with AVX (except MacOS?) and SSE enabled. When the PR is labeled for ARM then an extra runner for ARM is started.
Thus bugs in the scalar code-paths may not be covered by CI, as the path via AVX/SSE is taken usually.
Some bugs get revealed by running on ARM (e.g. #2409 (comment)) but CI should cover the scalar paths on non-ARM too.My suggestion is to add a new entry in the
thatImageSharp/.github/workflows/build-and-test.yml
Lines 16 to 19 in 302a23f
- e.g.
ubuntu-latest - has environment variable
DOTNET_EnableSSE=0set
so that JIT treatsSse.IsSupported = false(and all "inherited" intrinsics like SSE2, ..., AVX, AVX2), thus only the scalar code-paths are executed.
- e.g.
@brianpopow in #2413 (comment) you wrote about coverage.
Please check if there's coverage for
I guess that's not covered, as it's a scalar-path and methodClampedAddSubtractHalf(which calls this method) has a pure SSE2-implemation.We should be running any decoder/encoder tests using
FeatureTestRunnerwith which we can disable Avx/Sse etc.@brianpopow in #2413 (comment) you wrote about coverage.
Please check if there's coverage for
I guess that's not covered, as it's a scalar-path and method
ClampedAddSubtractHalf(which calls this method) has a pure SSE2-implemation.Here is the coverage report For LosslessUtils.
ClampedAddSubtractHalfis reported as covered, but theClip255which is called byClampedAddSubtractHalfis only reported as covered partially. I think that's the problem.
I will improve the test, so it will be fully covered.We should be running any decoder/encoder tests using FeatureTestRunner with which we can disable Avx/Sse etc.
There is a test
RunPredictor13Testwhich uses theFeatureTestRunnerto run with and without intrinsics. For context:Predictor13()callsClampedAddSubtractHalf().There are some paths not covered, for example
BundleColorMap()andSubtractGreenFromBlueAndRedScalar()Reacted by Günther FoidlThat's a bit strange. When the private method
AddSubtractComponentHalfis covered, and that method had the bug, then it should have shown up. Or is the test -- ran viaFeatureTestRunner-- only executing some inputs?
I'm new to the test infrastructure here, so I have to ask?Besides that I wonder why so many tests (10+) fail when SSE is disabled.
The issue is in
Clip255, this method is called byAddSubtractComponentHalf
and is not fully covered. Only the patha < 256 == trueis covered, not the casea < 256 == false
This hides the error. This means we need two test cases forRunPredictor13Testone fora < 256 == trueand one fora < 256 == false.
The when you revert back your commit b694956 to have the issue, it will show up as a failed test.FeatureTestRunnerallows us to run tests with disabled intrinsics features.
For example:FeatureTestRunner.RunWithHwIntrinsicsFeature( RunTest, count, HwIntrinsics.AllowAll | HwIntrinsics.DisableAVX2);This will run
RunTestwith hardware intrinsics once and once without AVX2 (assuming the host cpu has AVX2).Besides that I wonder why so many tests (10+) fail when SSE is disabled.
I had no chance yet to look into those failing tests, but we need to fix them all.
FeatureTestRunneris a nice solution. Thanks for the explanaition 👍🏻Is the cost of running of a whole CI-leg with SSE disabled too high?
Or can this be run only on a scheduled basis?
So all code-paths w/o vectorization are touched, and no one has to think about adding a test-case with the FeatureTestRunner.To cover all the intrinsic paths on 4 OS (linux is counted 2x now we have ARM) for 2X target frameworks the build matrix explodes to something like 28 images now.
FeatureTestRunnerwas my workaround to keep numbers low and also so we could get a detailed code coverage report from a single entry in the build matrix.I don't think there's a way to keep our coverage report intact while splitting out the features into separate matrix entries.
I would suggest we ensure that all decoder/encoder methods use
FeatureTestRunnerfor Avx, Sse, AdvancedSimd, and scalar runs. It's a chore but absolutely necessary.the build matrix explodes
This should be avoided, for sure.
I'd do only +1, so run a additionalubuntu-latestwithDOTNET_EnableSSE=0set. So that scalar pathes are touched at least once (for sure).
If there's a test missing for running viaFeatureTestRunnercode will be covered too.I'm not that familiar with GH-Actions (I'm using Azure Pipelines mostly) but I think that should be possible w/o exploding the matrix.
FeatureTestRunneris good and should be kept, as tests can be run on other operating systems too quite easily.
(BTW: your implementation of that is really nice).I can confirm this. I see 133 failing tests going scalar-only on my machine. For example, this is the output when decoding
testorig12.jpg:
I recommend fixing these one-by-one, adding the missing test cases for each individual bug in the process.
While
FeatureTestRunneris good tool, using it systematically in every single integration test would be very expensive. I think adding one test leg to cover the scalar case on one OS, as #2414 (comment) suggests, would be valuable.After merging #2416 many errors will be solved. Probably the image you have posted will render correct.
6 remaining items
All jpeg tests are passing now.
@br3aker thanks for looking into this issue!
Reacted by DmitryWith #2427 merged, 15 test remain failing on the current main branch now.
Two failing tests are webp encoding tests. I think there is an issue that the AVX version of
Vp8Residual.GetResidualCost()does not match the scalar version. I am trying to figure out what's wrong there.P.S. I whish there would be an option not to close linked issues when PR are merged. I could not find any.
The only way to avoid closing is not to use the closing keywords when linking to issues.
I cant figure out what's going wrong with those png encoder tests. As mentioned above those failing tests seem to be related to decoding text chunks.
Here is an example stack trace from
PngEncoderTests.Chunks.cs:Message: System.PlatformNotSupportedException : Operation is not supported on this platform. Stack Trace: Sse2.LoadScalarVector128(UInt64* address) Latin1Utility.WidenLatin1ToUtf16_Sse2(Byte* pLatin1Buffer, Char* pUtf16Buffer, UIntPtr elementCount) <>c.<GetString>b__29_0(Span`1 chars, ValueTuple`2 args) String.Create[TState](Int32 length, TState state, SpanAction`2 action) Latin1Encoding.GetString(Byte[] bytes) PngDecoderCore.TryUncompressTextData(ReadOnlySpan`1 compressedData, Encoding encoding, String& value) line 1385 PngDecoderCore.ReadCompressedTextChunk(ImageMetadata baseMetadata, PngMetadata metadata, ReadOnlySpan`1 data) line 1065 PngDecoderCore.Decode[TPixel](BufferedReadStream stream, CancellationToken cancellationToken) line 195 ImageDecoderUtilities.Decode[TPixel](IImageDecoderInternals decoder, Configuration configuration, Stream stream, Func`3 largeImageExceptionFactory, CancellationToken cancellationToken) line 57 ImageDecoderUtilities.Decode[TPixel](IImageDecoderInternals decoder, Configuration configuration, Stream stream, CancellationToken cancellationToken) line 43 PngDecoder.Decode[TPixel](DecoderOptions options, Stream stream, CancellationToken cancellationToken) line 38 <>c__DisplayClass0_0`1.<Decode>b__0(Stream s) line 23 ImageDecoder.<WithSeekableStream>g__PeformActionAndResetPosition|11_0[T](Stream s, Int64 position, <>c__DisplayClass11_0`1& ) line 194 ImageDecoder.WithSeekableStream[T](DecoderOptions options, Stream stream, Func`2 action) line 209 ImageDecoder.Decode[TPixel](DecoderOptions options, Stream stream) line 20 Image.Decode[TPixel](DecoderOptions options, Stream stream) line 122 <>c__DisplayClass84_0`1.<Load>b__0(Stream s) line 234 Image.WithSeekableStream[T](DecoderOptions options, Stream stream, Func`2 action) line 301 Image.Load[TPixel](DecoderOptions options, Stream stream) line 234 Image.Load[TPixel](DecoderOptions options, ReadOnlySpan`1 data) line 139 Image.Load[TPixel](ReadOnlySpan`1 data) line 120 TestFile.CreateRgba32Image() line 110 PngEncoderTests.Chunk_ComesBeforeIDat(Object chunkTypeObj) line 115The weird thing is, when I execute a single failing test, the test runs fine and is green.
Wild guess: maybe tiered compilation (rejitter?) doesn't respect ISA environmental variables on net6? Didn't check on net7.
I've set this env variable
DOTNET_TieredCompilation=0and got following results:
Last failing test is related to image diff threshold, not sure whether it's related to the codegen.
Now I'm completely lost, we can't simply set this variable and go on - there's a probability it can be broken by some R2R situation? Sorry to bother you @tannergooding but I'm not sure we can solve this on our own :)
Could I get a few more details on exactly what's being set and what OS/architecture you're running on?
what OS/architecture you're running on
Win11/x86, no emulation
what's being set
We are trying to test all ImageSharp tests for scalar path so I've set
<DOTNET_EnableSSE>0</DOTNET_EnableSSE>in.runsettingsfor emulating cpu without sse support at all.We got some flaky tests which end up throwing
System.PlatformNotSupportedException : Operation is not supported on this platform.. If these tests are executed among other tests - everything breaks due to this call:Sse2.LoadScalarVector128(UInt64* address)frommscorlib, you can check exact stacktrace from the message above mine.
But if these flacky tests are launched in isolation - they successfully pass. I immediately thought about tiered jitting and decided to turn it off with<DOTNET_TieredCompilation>0</DOTNET_TieredCompilation>in.runsettingsfile, file looked like this:<?xml version="1.0" encoding="utf-8" ?> <RunSettings> <RunConfiguration> <DOTNET_EnableSSE>0</DOTNET_EnableSSE> <DOTNET_TieredCompilation>0</DOTNET_TieredCompilation> </RunConfiguration> </RunSettings>With this settings flaky tests do not fail, i.e. no more
System.PlatformNotSupportedExceptionfrom SSE calls.Reacted by Brian PopowThis sounds like a bad interaction between
EnableSSE=0andReadyToRun. Does settingDOTNET_ReadyToRun=0fix it?Yep, setting
DOTNET_ReadyToRun=0fixed tests. Should we open an issue or it's intended/workaround exists?Please open an issue. I'm pretty sure I know what the issue is, but would be good to get it tracked explicitly as impacting the broader community
Got it, will do tomorrow, thank you for the help!
Reacted by Tanner Gooding, Brian Popow and Anton FirszovI think we can go ahead and close this now that the upstream issue is fixed in .NET 7+
Prerequisites
DEBUGandRELEASEmodeImageSharp version
local build with latest main-branch
Other ImageSharp packages and versions
none
Environment (Operating system, version and so on)
win-x64, DOTNET_EnableSSE=0
.NET Framework version
all
Description
While hunting the bug (fixed via #2413) I tried several things, amongst others disabling SSE for unit tests run with Visual Studio by using this
ImageSharp/.runsettings
Line 2 in 0a1f05b
Tests started to fail -- even when going back in the history of main-branch (so no recent commit caused these failures).
To me it looks like there are latent bugs in the scalar code-pathes. Can anyone confirm?
(description copied and adapted from #2413 (comment))
Steps to Reproduce
To reproduce together with that runsettings one can try this playlist.zip (for filtered tests).
Images
No response