BenchmarkDotNet icon indicating copy to clipboard operation
BenchmarkDotNet copied to clipboard

Benchmark(Baseline = true) doesn't work when using byte[] arrays in Params()

Open IanKemp opened this issue 7 years ago • 6 comments

    public class ToBase64UrlEncodedString
    {
        [Params(new byte[] { 57, 48, 0, 0, 1, 39, 2, 0, 0, }, new byte[] { 57, 48, 0, 0, 0, 0, 0, 0, 0, })]
        public byte[] From { get; set; }

        [Benchmark(Baseline = true)]
        public string Convoluted()
        {
            return ImageUrlEncryptor.ToBase64UrlEncodedString(From);
        }

        [Benchmark]
        public string Microsoft_IdentityModel_Tokens()
        {
            return ImageUrlSafe.ToBase64UrlEncodedString(From);
        }
    }

Results in:

C:\Working\Console2\Console2\bin\Release> .\Console2.exe
// ***** BenchmarkRunner: Start   *****
// Found benchmarks:
//   ToBase64UrlEncodedString.Convoluted: InProcess(Toolchain=InProcessToolchain [From=System.Byte[]]
//   ToBase64UrlEncodedString.Convoluted: InProcess(Toolchain=InProcessToolchain) [From=System.Byte[]]
//   ToBase64UrlEncodedString.Microsoft_IdentityModel_Tokens: InProcess(Toolchain=InProcessToolchain) [From=System.Byte[]]
//   ToBase64UrlEncodedString.Microsoft_IdentityModel_Tokens: InProcess(Toolchain=InProcessToolchain) [From=System.Byte[]]

// Validating benchmarks:
Only 1 benchmark method in a group can have "Baseline = true" applied to it, group InProcess(Toolchain=InProcessToolchain)-[From=System.Byte[]] in class ToBase64UrlEncodedString has 2
// * Artifacts cleanup *

Is this a supported scenario?

IanKemp avatar Jun 11 '18 06:06 IanKemp

I found the problem. Currently, ArrayParam<T>.DisplayText returns $"Array[{array.Length}]". It means that arrays of equal size have the same DisplayText. We have two problems here:

  • The benchmarks have the same logical group keys, so baselining is broken.
  • These benchmarks are indistinguishable in the logs/summary.

I think it makes sense to rewrite ArrayParam<T>.DisplayText and return a unique string. Since we can't use the full content of the array (it can be very huge), we can use the absolute position of the array across all values. @adamsitnik, what do you think?

AndreyAkinshin avatar Jun 11 '18 09:06 AndreyAkinshin

@IanKemp, currently, you can rewrite your benchmark as follows:

public class ToBase64UrlEncodedString
{
    [Benchmark(Baseline = true)]
    [ArgumentsSource(nameof(Data))]
    public string Convoluted(string name, byte[] from)
    {
        return ImageUrlEncryptor.ToBase64UrlEncodedString(from);
    }

    [Benchmark]
    [ArgumentsSource(nameof(Data))]
    public string Microsoft_IdentityModel_Tokens(string name, byte[] from)
    {
        return ImageUrlSafe.ToBase64UrlEncodedString(from);
    }

    public IEnumerable<object[]> Data()
    {
        yield return new object[]
        {
            "FirstArray", 
            new byte[] { 57, 48, 0, 0, 1, 39, 2, 0, 0 }
        };
        yield return new object[]
        {
            "SecondArray", 
            new byte[] { 57, 48, 0, 0, 0, 0, 0, 0, 0 }
        };
    }
}

We need name here to distinguish arrays of equal size.

AndreyAkinshin avatar Jun 11 '18 09:06 AndreyAkinshin

We need name here to distinguish arrays of equal size.

I agree but I don't have a good idea for unique, meaningful but short display name

adamsitnik avatar Jun 11 '18 11:06 adamsitnik

@adamsitnik, if we are talking about arrays, we can add something like $1, $2, etc. to the name where the number is the absolute position of the array. It will be meaningful, short, and unique.

AndreyAkinshin avatar Jun 11 '18 11:06 AndreyAkinshin

@AndreyAkinshin good idea!

adamsitnik avatar Jun 11 '18 12:06 adamsitnik

Currently the bug is different - arrays are not compared properly.

OP example displays:

                         Method |    From |     Mean |   Error |  StdDev | Ratio | RatioSD |                    LogicalGroup |
------------------------------- |-------- |---------:|--------:|--------:|------:|--------:|-------------------------------- |
                     Convoluted | Byte[9] | 102.0 ns | 6.09 ns | 1.58 ns |  1.00 |    0.00 | [From=System.Byte[]]-DefaultJob |
                     Convoluted | Byte[9] | 202.0 ns | 6.09 ns | 1.58 ns |  1.98 |    0.02 | [From=System.Byte[]]-DefaultJob |
 Microsoft_IdentityModel_Tokens | Byte[9] | 302.0 ns | 6.09 ns | 1.58 ns |  2.96 |    0.03 | [From=System.Byte[]]-DefaultJob |
 Microsoft_IdentityModel_Tokens | Byte[9] | 402.0 ns | 6.09 ns | 1.58 ns |  3.94 |    0.05 | [From=System.Byte[]]-DefaultJob |

The PR changes it to:

                         Method |    From |     Mean |   Error |  StdDev | Ratio | RatioSD |                           LogicalGroup |
------------------------------- |-------- |---------:|--------:|--------:|------:|--------:|--------------------------------------- |
                     Convoluted | Byte[9] | 102.0 ns | 6.09 ns | 1.58 ns |  1.00 |    0.00 |  [From=57,48,0,0,0,0,0,0,0]-DefaultJob |
 Microsoft_IdentityModel_Tokens | Byte[9] | 202.0 ns | 6.09 ns | 1.58 ns |  1.98 |    0.02 |  [From=57,48,0,0,0,0,0,0,0]-DefaultJob |
                                |         |          |         |         |       |         |                                        |
                     Convoluted | Byte[9] | 302.0 ns | 6.09 ns | 1.58 ns |  1.00 |    0.00 | [From=57,48,0,0,1,39,2,0,0]-DefaultJob |
 Microsoft_IdentityModel_Tokens | Byte[9] | 402.0 ns | 6.09 ns | 1.58 ns |  1.33 |    0.00 | [From=57,48,0,0,1,39,2,0,0]-DefaultJob |

I didn't add postfix in this PR. If a postfix is desirable, then do not close this issue and let me know.

YegorStepanov avatar Oct 16 '22 16:10 YegorStepanov