[Proposal]: Allow "null" default value for "params" parameters

[Chakrygin]

Allow "null" default value for "params" parameters

• Proposed
• Prototype: Not Started
• Implementation: Not Started
• Specification: Not Started

Summary

When calling such methods with an empty argument list, use null instead of an empty array.

Motivation

Now, if the developer defines a method with the "params" keyword:

public void Test(params string[] values)
{
    // ...
}

And then the method is called with no arguments:

Test();

This code is equivalent to:

var values = Array.Empty<string>();
Test(values);

Although this code does not allocate additional memory, it has a small overhead of getting an empty array.
Therefore, many developers explicitly overload such methods:

public static void Test()
{
    Test(null);
}

This is a proposal to be able to use the null default instead of the overloads, like this:

public void Test(params string[] values = null)
{
    // ...
}

Detailed design

Drawbacks

Alternatives

Unresolved questions

Design meetings

[YairHalberstadt]

Do you have any benchmarks as to what this overhead actually is? I would be shocked if this actually mattered.

[RikkiGibson]

seems like this saves a call to System.Array.Empty(). SharpLab

[YairHalberstadt]

@RikkiGibson that should end up being a single access to a static read-only field. I would be surprised if there were any apps where that mattered.

[Chakrygin]

I have tried to make the simple benchmark:

[SimpleJob(launchCount: 1, warmupCount: 10, targetCount: 100, invocationCount: 20000000)]
public class BenchmarkClass
{
    [Benchmark]
    public string[] NullValue()
    {
        return Test(null);
    }

    [Benchmark]
    public string[] EmptyArrayValue()
    {
        return Test();
    }

    public static string[] Test(params string[] value)
    {
        return value;
    }
}

I have got the following results on my machine:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
Intel Core i5-2320 CPU 3.00GHz (Sandy Bridge), 1 CPU, 4 logical and 4 physical cores
.NET Core SDK=5.0.102
  [Host]     : .NET Core 5.0.2 (CoreCLR 5.0.220.61120, CoreFX 5.0.220.61120), X64 RyuJIT
  Job-FYYVGK : .NET Core 5.0.2 (CoreCLR 5.0.220.61120, CoreFX 5.0.220.61120), X64 RyuJIT

InvocationCount=20000000  IterationCount=100  LaunchCount=1  
WarmupCount=10  

|          Method |      Mean |     Error |    StdDev |    Median |
|---------------- |----------:|----------:|----------:|----------:|
|       NullValue | 0.0276 ns | 0.0127 ns | 0.0368 ns | 0.0032 ns |
| EmptyArrayValue | 0.0532 ns | 0.0218 ns | 0.0643 ns | 0.0206 ns |

[theunrepentantgeek]

Three observations.

Firstly, the difference in performance between those two calls is so small as to be meaningless for any real world situation. It's a trivial per-call overhead, and you'd need a tight loop executing a billion calls before it scaled up enough to be human perceivable.

Secondly, the actual difference is even less than your benchmark shows, because now the implementation needs to test for, and handle, being passed a null value. You've got the overhead of the null test itself, plus any potential overhead from a branch predication failure by the CPU. This very well might tip the balance the other way.

Thirdly, a compiler change to pass null instead of Array.Empty<string>() would be a breaking change that would cause existing code to throw a NullReferenceException.

The LDM has a very high threshold for a breaking change to be accepted. Such a change has to be shown that the pain caused by the problem is substantially higher than the pain that would be caused by the fix.

[theunrepentantgeek]

You'd nabbed my curiosity @Chakrygin so I tried out running the benchmarks on my own machine.

My first change was to change all the methods to return int instead of string[]:

|          Method |      Mean |     Error |    StdDev |    Median |
|---------------- |----------:|----------:|----------:|----------:|
|       NullValue | 0.0192 ns | 0.0084 ns | 0.0236 ns | 0.0093 ns |
| EmptyArrayValue | 0.0030 ns | 0.0038 ns | 0.0107 ns | 0.0000 ns |

Interesting to note that now EmptyArrayValue seems to be 6x faster, but with very large values for the Standard Deviation of those measurements.

My second change was to change Test to return the length of the parameter array, defaulting to 0 if null passed:

public static int Test(params string[] value)
{
    if (value == null)
    {
        return 0;
    }

    return value.Length;
}

|          Method |      Mean |     Error |    StdDev | Median |
|---------------- |----------:|----------:|----------:|-------:|
|       NullValue | 0.0015 ns | 0.0020 ns | 0.0057 ns | 0.0 ns |
| EmptyArrayValue | 0.0017 ns | 0.0027 ns | 0.0075 ns | 0.0 ns |

Here we see NullValue is faster, but only by a nose. Once the standard deviation of those measurements is taken into account, the differences are effectively zero.

[333fred]

Don't forget the margins of error in those tests. In every benchmark that has been posted to this thread so far, nothing can be gleaned about which is faster because the times +/- the error has overlapped.

[333fred]

Also, benchmark.net has a statistical test mode for telling you whether a difference is meaningful:

  --statisticalTest        Threshold for Mann–Whitney U Test. Examples: 5%, 
                           10ms, 100ns, 1s

[IS4Code]

@theunrepentantgeek This proposal does not introduce breaking changes. params string[] values = null is currently not valid definition of a parameter, so allowing it would not break existing code.

[Cologler]

The test did not work because Test(null) equals Test(new string[] { null }).

[IS4Code]

@Cologler It does not – if params can be ignored, it is (if the value is assignable to the actual array type).

[Cologler]

@IllidanS4 commented on 2021年1月31日 GMT+8 下午7:20:

@Cologler It does not – if params can be ignored, it is (if the value is assignable to the actual array type).

You are right. This is interesting and the document only implicitly mentions it. (╯°□°）╯︵ ┻━┻

[huoyaoyuan]

Null test may be more annoying than passing a dummy reference.

Anyway, params is not for performance. It should be handled by potential params Span, and acquiring of empty span is totally free. Null can be safely treated as empty for span.

[john-h-k]

Firstly, the difference in performance between those two calls is so small as to be meaningless for any real world situation. It's a trivial per-call overhead, and you'd need a tight loop executing a billion calls before it scaled up enough to be human perceivable.

Human perceivable is not a relevant benchmark timescale for many things

[john-h-k]

The more meaningful overhead of this is that Array.Empty<T>() calls trigger a generic type instanitation. There are several places the BCL avoids calling it purely for this reason, as it can increase memory non trivially
For example:

This is a small overhead admittedly, but worth mentioning

[HaloFour]

@john-h-k

That's ImmutableArray.Empty, which is a different beast. Array.Empty just returns a singleton instance of an empty array which should be inlined.

SharpLab

[john-h-k]

@HaloFour yes, but it was an example of explicitly not using Array.Empty (it uses new T[0] instead) because calling Array.Empty may trigger a type instantiation

There are several other examples, possibly most notably in List<T>:

ArraySegment

SZArrayEnumerator

[IS4Code]

In all those cases, it's an array that may not be ever needed. Use List<T> with 100 types and you get 100 instantiations; use Array.Empty inside and you get 200 instantiations, half of which you don't need. The BCL is the kind of place where this does matter.

In method calls, the result of the instantiation is actually used.

[HaloFour]

@john-h-k

Those are all cases within another generic type, though. And IIRC that's a cost you only incur once.

[john-h-k]

Yes, but nonetheless it shows why OP's argument is at least somewhat valid, just not because of speed

[CyrusNajmabadi]

I don't currently see any value demonstrated in this proposal. This feels like a case of "if it ain't broke". I'm also moving this to be a discussion as per our policies on issues being used officially for actual bugs or for taking proposals that we actually intend to take through the LDM process.