Adding span tests to improve coverage

[ahsonkhan]

Fixes #17078

Adding tests for:

• Implicit operators
• Obsolete methods
• Clear EnumType and Clear ValueType Without References Pointer Size
• Copy empty source
• Fill native memory
• Fill ValueType Without References for larger arrays
• Test IndexOf for larger arrays

Gets us much closer to 100% test coverage (for slow span at least), esp if you include outer-loop and 32-bit tests.

[ahsonkhan]

cc @shiftylogic, @KrzysztofCwalina, @jkotas

[ahsonkhan]

@dotnet-bot test code coverage please

[ahsonkhan]

Questions:

• How do we get branch coverage whereif (Vector.IsHardwareAccelerated) is false? This is always true for our tests.

• @benaadams, when is this true (i.e. what test scenario would expose this branch)?
  https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/SpanHelpers.byte.cs#L144

• Is there a way to prevent generation and compilation of System.SR (i.e. SR.cs) for fast span by excluding Strings.resx?

[ahsonkhan]

@dotnet-bot test Innerloop OSX10.12 Debug x64 Build and Test please

[benaadams]

How do we get branch coverage whereif (Vector.IsHardwareAccelerated) is false? This is always true for our tests.

Use #if instead and use readonly static instead of Vector.IsHardwareAccelerated?

#if DEBUG
    readonly static bool IsHardwareAccelerated = false;
#else
    readonly static bool IsHardwareAccelerated = Vector.IsHardwareAccelerated;
#end

when is this true

Input is multiple of Vector length without match

[ahsonkhan]

Use #if instead and use readonly static instead of Vector.IsHardwareAccelerated?

I do not want to change the source. I am wondering if there is a change on the test infra side that would test this branch. Do we run CI on machines where this is false?

[ahsonkhan]

Input is multiple of Vector length without match

I tried that with the following test, but I don't hit that branch:

corefx/src/System.Memory/tests/Span/IndexOf.byte.cs

Line 58 in f77be4b

public static void TestNoMatch_Byte()

[benaadams]

Might be alignment adjustment as the array is allocated each time and will move in memory.

Allocate an fixed array then for loop down it to move the alignment?

e.g. something like

byte[] array = new byte[4 * Vector<byte>.Count];

for (var i = 0; i < array.Length - Vector<byte>.Count; i++)
{
    var span = new Span<byte>(array, i, Vector<byte>.Count);
    int idx = span.IndexOf((byte)'1');
    Assert.Equal(-1, idx);
}

[ahsonkhan]

Might be alignment adjustment as the array is allocated each time and will move in memory.

This doesn't hit it either. Also, in your example code, the span length is always Vector.Count which means you always drop down to SequentialScan (i.e., this is false):

corefx/src/System.Memory/src/System/SpanHelpers.byte.cs

Line 59 in f77be4b

if (Vector.IsHardwareAccelerated && length >= Vector<byte>.Count * 2)

I tried the following and still ((int)(byte*)index > length) is always false.

            byte[] array = new byte[6 * Vector<byte>.Count];

            for (var i = 0; i < array.Length - 3* Vector<byte>.Count; i++)
            {
                var span = new Span<byte>(array, i, 3* Vector<byte>.Count);
                int idx = span.IndexOf((byte)'1');
                Assert.Equal(-1, idx);
            }

[ahsonkhan]

@dotnet-bot test code coverage please

[benaadams]

:(

[ahsonkhan]

@benaadams, is this unnecessary/dead code then?

@shiftylogic, does this PR look good?

[benaadams]

Would suggest an unnecessary jump back; might need to be if (index == length) instead

[AndyAyersMS]

You can disable vectorization by setting COMPlus_FeatureSIMD=0 before you run tests. This should cause the IsHardwareAccelerated predicate to evaluate to false.

You can force a shorter vector lengths (assuming you normally run on AVX capable HW) via COMPlus_EnableAVX=0.

Both of these are available in release builds, I believe.

[ahsonkhan]

You can disable vectorization by setting COMPlus_FeatureSIMD=0 before you run tests. This should cause the IsHardwareAccelerated predicate to evaluate to false.

@AndyAyersMS, thanks :)
Is this a change in the environment variable or can you do this in C# code? If so, can you provide code samples where this is done?

[ahsonkhan]

@dotnet-bot test code coverage please

[ahsonkhan]

Would suggest an unnecessary jump back; might need to be if (index == length) instead

@benaadams, I submit a PR with changes to the IndexOf worker method, please take a look: #17500

[AndyAyersMS]

It's an environment setting. It needs to be determined when the CLR initializes, before any jitting happens, since it alters the way SIMD types are handled. You can't set this from C# code.

[ahsonkhan]

It's an environment setting. It needs to be determined when the CLR initializes, before any jitting happens, since it alters the way SIMD types are handled. You can't set this from C# code.

So how can we add this to CI?

[ahsonkhan]

@dotnet-bot test Innerloop Windows_NT Debug x86 Build and Test please

[AndyAyersMS]

There might be some sort of customizable environment setup that you could leverage to get the right settings in place before running tests. CoreCLR's CI has this for things like jit stress and gc stress.

[ahsonkhan]

There might be some sort of customizable environment setup that you could leverage to get the right settings in place before running tests.

Who would be the right person to talk to about making this change? @crummel

[ahsonkhan]

@shiftylogic, good to go?

[shiftylogic]

👍

[ahsonkhan]

Fixes part of dotnet/corefxlab#1314