Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Champion "params Span<T>" #1757

Closed
1 of 5 tasks
gafter opened this issue Jul 31, 2018 · 73 comments
Closed
1 of 5 tasks

Champion "params Span<T>" #1757

gafter opened this issue Jul 31, 2018 · 73 comments

Comments

@gafter
Copy link
Member

gafter commented Jul 31, 2018

  • Proposal added
  • Discussed in LDM
  • Decision in LDM
  • Finalized (done, rejected, inactive)
  • Spec'ed

As suggested by @alrz in #1412 (comment), we could permit params Span<T> to implement params parameter-passing without any heap allocation. This could make the use of params methods much more efficient.

Proposal: https://github.com/dotnet/csharplang/blob/main/proposals/params-span.md

LDM history:

@gafter gafter self-assigned this Jul 31, 2018
@alrz
Copy link
Contributor

alrz commented Jul 31, 2018

Previous discussion: #535

@4creators
Copy link

Similar issue which could be embraced by this proposal #1758

@gafter
Copy link
Member Author

gafter commented Aug 3, 2018

This would not work with the current CLR spec, as stackalloc'ed memory cannot contain references.

@alrz
Copy link
Contributor

alrz commented Aug 3, 2018

This would not work with the current CLR spec, as stackalloc'ed memory cannot contain references.

We could fallback to Span(T[]) in that case (or basically, anywhere stackalloc is forbidden).

@alrz
Copy link
Contributor

alrz commented Aug 7, 2018

This ones mentioned in the prior thread,

SomeType Foo(params int[] values);
SomeType Foo(params Span<int> values);

As discussed before, we don't need to define any resolution precedence here, you can just move params to the span overload to make it preferred without incurring any breaking changes.

That being said, I propose to disallow such overloads because almost all invocations would be ambiguous and the overload resolution as-is wouldn't make it an error for "free" (in the expanded form).

@gafter
Copy link
Member Author

gafter commented Aug 7, 2018

... I propose to disallow such overloads ...

What kind of rule did you have in mind for that?

@scalablecory
Copy link

... I propose to disallow such overloads ...

I would prefer to handle this in the params expansion rules than in overload restrictions. Have it explicitly expand into a Span if available, otherwise into an array/enumerable.

This makes more sense to me because from a caller's standpoint just calling Foo(1, 2, 3), the params are "containerless" and I'd expect the compiler to expand into the most optimal choice. And importantly, it would also provide a simple upgrade path for current APIs taking arrays: supply the new API, recompile the code using that API, and everything gets faster without extra work.

@gafter
Copy link
Member Author

gafter commented Aug 7, 2018

@scalablecory The params expansion rule in https://github.com/dotnet/csharplang/blob/master/spec/expressions.md#applicable-function-member makes the underlying type of the params parameter (array or span) vanish for the purposes of overload resolution, so I think that doesn't work.

The expanded form is constructed by replacing the parameter array in the function member declaration with zero or more value parameters of the element type of the parameter array such that the number of arguments in the argument list A matches the total number of parameters. If A has fewer arguments than the number of fixed parameters in the function member declaration, the expanded form of the function member cannot be constructed and is thus not applicable.

@alrz
Copy link
Contributor

alrz commented Aug 8, 2018

@gafter

params parameters with identical underlying types would give you a conflicting overload error, regardless of it being array or span (e.g. int in the example above). This is applicable to params IEnumerable<T> as well.

@alrz
Copy link
Contributor

alrz commented Aug 8, 2018

@scalablecory

I would prefer to handle this in the params expansion rules than in overload restrictions. Have it explicitly expand into a Span if available, otherwise into an array/enumerable.

As I said, there's no need to have a preference (if at all possible), for instance, if you have

void Foo(params int[] values);

You can add an overload and move the params,

void Foo(params Span<int> values);
void Foo(int[] values);

This is still binary-compatible with existing code, and a recompilation would resolve all those invocations to the new overload.

Downside of this approach is that you must upgrade your compiler to be able to use such APIs. That doesn't mean that we need to bump up the lang version, we can continue to expand to span at the call-site even with prior lang versions.

@popcatalin81
Copy link

This would be another great case for #1447 Proposal: Customisable overload resolution

@svick
Copy link
Contributor

svick commented Oct 4, 2018

Would the upcoming work on object stack allocation in CoreCLR (see dotnet/coreclr#20251) make this feature less important?

@alrz
Copy link
Contributor

alrz commented Oct 4, 2018

@svick wow didn't know that's a thing. I think that would permit using stackalloced Span storage also with references?

@alrz
Copy link
Contributor

alrz commented Oct 4, 2018

Would the upcoming work on object stack allocation in CoreCLR (see dotnet/coreclr#20251) make this feature less important?

I should note that due to required escape analysis on stackalloced memory, this can't be applied to regular params arrays, if that's what you're implying.

@svick
Copy link
Contributor

svick commented Oct 4, 2018

@alrz Why not? The params array usually does not escape the called function, so escape analysis should figure out that the array can be stack allocated.

@popcatalin81
Copy link

popcatalin81 commented Oct 4, 2018

Stack alloc would be great for params array.

Expecially the following BCL methods would likely benefit significantly:

String.Concat (2 overloads)
String.Format (2 overloads)
String.Join (2 overloads)
String.Split (1 overloads)
String.Trim (1 overloads)
String.TrimEnd (1 overloads)
String.TrimStart (1 overloads)
StringBuilder.AppendFormat (2 overloads)
Task.WaitAll (1 overloads)
Task.WaitAny (1 overloads)
Task.WhenAll (2 overloads)
Task.WhenAny (2 overloads)
TextWriter.Write (4 overloads)
TextWriter.WriteLine (4 overloads)

Maybe the Jit can smart and only do stackalloc for arrays bellow a certain size, but it would definatelly be usefull.

@alrz
Copy link
Contributor

alrz commented Oct 4, 2018

The params array usually does not escape the called function.

@svick there's no restriction on the callee, it could freely leak out the array instance. That's not true of Span.

@DarthVella
Copy link

@alrz I don't follow - whether or not the function allows the array to escape and the object accordingly allowed to allocate on the stack, or not, would be deduced on a case-by-case basis. Although, personally, I agree; that decision should be reinforced by explicitly using a Span parameter.

@gafter gafter added this to 8.0 Candidate (not started) in Language Version Planning Mar 6, 2019
@MadsTorgersen MadsTorgersen moved this from 8.0 Candidate (not started) to 9.0 Candidate in Language Version Planning Apr 29, 2019
@HaloFour
Copy link
Contributor

@TahirAhmadov

See: #179

@RikkiGibson
Copy link
Contributor

it feels like scoped is the concept we needed here. Basically, if a params Span<T> is implicitly also scoped, then we know the caller can't let it escape anywhere, and that it's fine for us to keep reusing the same stackalloc'd memory across different calls.

@jaredpar
Copy link
Member

@RikkiGibson

100% agree. I've had a few chats since last time I posted on this issue. The conclusion we came to was exactly as you suggested that we need scoped. Effectively I think that params ReadOnlySpan<int> compiles down to scoped ReadOnlySpan<int>. Essentially params attached to any ref struct collection is implicitly scoped.

@TahirAhmadov
Copy link

I think we should not rush on this and try to come up with a solution which includes passing in Span<T> and IEnumerable<T>/other collection types.

@abnercpp
Copy link

abnercpp commented Apr 7, 2023

It would need only a warning if the compiler is forced to implement the span creation via stackalloc inside that loop (stack allocated memory is not freed after an iteration ...). But there are way more better ways to implement params Span<T>, e.g.

(And of course for static / const content we could use memcopy...)

"Stackalloc" via value type (like written in the proposal)

using System;
using System.Runtime.InteropServices;

int count = 10;
for(int i = 0; i < count; i++)
{
    var data = new LoweredSpanStorage();
    var dataSpan = MemoryMarshal.CreateSpan<int>(
        ref System.Runtime.CompilerServices.Unsafe.AsRef<int>(data.data),
        10
    );
    dataSpan[0] = 0;
    dataSpan[1] = 1;
    dataSpan[2] = 2;
    dataSpan[3] = 3;
    dataSpan[4] = 4;
    dataSpan[5] = 5;
    dataSpan[6] = 6;
    dataSpan[7] = 7;
    dataSpan[8] = 8;
    dataSpan[9] = 9;
    Test(dataSpan);
}

void Test(Span<int> span)
{
    foreach(var s in span)
    {
        Console.Write(s);
    }
    Console.WriteLine();
}

[StructLayout(LayoutKind.Sequential, Size=10 * 4)] //size of element is 4 Byte - padding is 0
public struct LoweredSpanStorage
{
    public byte data;
}

Via ArrayPool:

using System;

int count = 10;
for(int i = 0; i < count; i++)
{
    var array = System.Buffers.ArrayPool<int>.Shared.Rent(10);
    var dataSpan = new Span<int>(array);
    try {
        dataSpan[0] = 0;
        dataSpan[1] = 1;
        dataSpan[2] = 2;
        dataSpan[3] = 3;
        dataSpan[4] = 4;
        dataSpan[5] = 5;
        dataSpan[6] = 6;
        dataSpan[7] = 7;
        dataSpan[8] = 8;
        dataSpan[9] = 9;
        Test(dataSpan);    
    } finally {
        System.Buffers.ArrayPool<int>.Shared.Return(array);
    }
}

void Test(Span<int> span)
{
    foreach(var s in span)
    {
        Console.Write(s);
    }
    Console.WriteLine();
}

or via non simple stackalloc:

using System;

int count = 10;
Span<int> dataSpan = stackalloc int[10];
for(int i = 0; i < count; i++)
{
    dataSpan[0] = 0;
    dataSpan[1] = 1;
    dataSpan[2] = 2;
    dataSpan[3] = 3;
    dataSpan[4] = 4;
    dataSpan[5] = 5;
    dataSpan[6] = 6;
    dataSpan[7] = 7;
    dataSpan[8] = 8;
    dataSpan[9] = 9;
    Test(dataSpan);
}

void Test(Span<int> span)
{
    foreach(var s in span)
    {
        Console.Write(s);
    }
    Console.WriteLine();
}

But all these are implementation details - in my eyes it would be even legal to create an array like we did before (it has just less performance benefits). params Span<T> simply allows more optimizations (because of the "safe to escape rules").

It would be interesting if the compiler could determine whether to stackalloc or rent from the shared array pool based on the number of arguments, and then take care of proper disposal of the array if we do rent one. Span<T> does not care whether the memory comes from the stack or the heap.

@KalleOlaviNiemitalo
Copy link

Re #1757 (comment)

var array = System.Buffers.ArrayPool<int>.Shared.Rent(10);
var dataSpan = new Span<int>(array);

Because ArrayPool<T>.Rent(int minimumLength) can return an array larger than requested, this would have to do new Span<int>(array, 0, 10).

@Tinister
Copy link

My understanding is the team no longer has an appetite for a params IEnumerable<T> (or similar) feature, due to the imminent release of collection literals. Call sites can instead just write Foo([0, 1, x, y]) instead of changing the signature (or adding an overload) to add params.

And I actually agree with this. Collection literals handles this nicely.

But then why params Span<T>? Especially since collection literals will target spans.

Or to ask a more pointed question, what's the difference between these two call sites (and when should class designers choose to add params or not)?

// void M(params Span<int> values);
M(0, 1, x, y);
// void N(Span<int> values);
N([0, 1, x, y]);

@CyrusNajmabadi
Copy link
Member

But then why params Span? Especially since collection literals will target spans.

Because then you get the perf benefit just by recompiling. You don't need to update any of your own code. It will be an automatic speedup for literally billions of lines of code just from the upgraded .net and compiler alone.

Params span gets you that. :-)

@Tinister
Copy link

So with regards to my second question(s), the N callsite will be less performant (won't be a stackalloc?)?

And class designers should always try to define params Span<T> when they can (or at least in performance-sensitive contexts)?

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jul 23, 2023

So with regards to my second question(s), the N callsite will be less performant (won't be a stackalloc?)?

If it is a scoped parameter then they will be equal. Params Span is implied 'scoped'.

And class designers should always try to define params Span when they can (or at least in performance-sensitive contexts)?

That would make sense to me as long as they can do something special with the Span. For example, if they actually capture the array, then there would be no point.

@Neme12
Copy link

Neme12 commented Jul 23, 2023

But then why params Span<T>? Especially since collection literals will target spans.

My understanding is that N([0, 1, x, y]) won't be possible if the method has overloads for both Span<T> and another collection type (e.g. an array or IEnumerable<T>).

@CyrusNajmabadi
Copy link
Member

Note: whatever tiebreak rule we will have for params we will get likely carry to collection expressions. A core design goal is that they feel and behave the same.

@Neme12
Copy link

Neme12 commented Jul 24, 2023

Oh, I see the proposal now: #7276

@hez2010
Copy link

hez2010 commented Aug 6, 2023

This can be easily implemented once we have const generics support in IL and a managed type struct ValueArray<T, int Length> to allow you to stack allocate an array of arbitrary types (including generic types).

void Test<T>(T x, T y, T z, T w)
{
    Use(x, y, z, w);
}

void Use<T>(params Span<T> x) { }
struct Bar { }

where Use(x, y, z, w) got lowered to:

var arr = new ValueArray<T, 4>();
arr[0] = x;
arr[1] = y;
arr[2] = z;
arr[3] = w;
Use(arr.AsSpan());

See dotnet/runtime#89730 for details.

We already have a fully working MVP implementation of const generics and may introduce const generics formally in .NET 9. So please hold on :)

@HaloFour
Copy link
Contributor

HaloFour commented Aug 6, 2023

@hez2010

We already have a fully working MVP implementation of const generics and may introduce const generics formally in .NET 9. So please hold on :)

If that's going to be a thing I wonder if the LDG should consider how inline arrays might be able to eventually migrate to this feature.

@Enderlook
Copy link

Enderlook commented Aug 6, 2023

This can be easily implemented once we have const generics support in IL and a managed type struct ValueArray<T, int Length> to allow you to stack allocate an array of arbitrary types (including generic types).

void Test<T>(T x, T y, T z, T w)
{
    Use(x, y, z, w);
}

void Use<T>(params Span<T> x) { }
struct Bar { }

where Use(x, y, z, w) got lowered to:

var arr = new ValueArray<T, 4>();
arr[0] = x;
arr[1] = y;
arr[2] = z;
arr[3] = w;
Use(arr.AsSpan());

See dotnet/runtime#89730 for details.

We already have a fully working MVP implementation of const generics and may introduce const generics formally in .NET 9. So please hold on :)

You don't need to wait for const generics.

As long as it's an implementation detail (so it can be changed later), it could be:

var arr = new UnspeakableBy4<T>();
arr[0] = x;
arr[1] = y;
arr[2] = z;
arr[3] = w;
Use(arr.AsSpan());

[InlineArray(4)]
file struct UnspeakableBy4<T>
{
    public T field0;
    public Span<T> AsSpan() => /* ... */;
}

It may look weird to create a type just for that... but the compiler already creates types for anonymous delegates and fixed buffers, so one more case is not the end of the world.

Not sure how flexible Roslyn policy is about changing under-the-hood implementations of features but just saying this could be a possibility. Thought... on the other hand this could also be done without InlineArray years ago and it wasn't, so maybe waiting one more year is fine I guess?

@CyrusNajmabadi
Copy link
Member

We don't need to wait here at all. Compiler can generate whatever code is suitable depending on what the runtime and bcl make available. Initially, we'll use InlineArray (and we will synthesize the types if necessary). If a better option comes along later, the compiler can switch to it then.

@Neme12
Copy link

Neme12 commented Aug 6, 2023

Just curious: why does the compiler need to use a struct at all? Why not either use stackalloc or just have a bunch of variables that are next to each other and create a span over them?

@CyrusNajmabadi
Copy link
Member

Just curious: why does the compiler need to use a struct at all? Why not either use stackalloc

Stackalloc only works for unmanaged types. So you could not use it for most of the types you want to pass as params Span. Also, stackalloc, well, allocs. Each time you hit it out will allocate more stack space. So, for example, if you have a foreach loop, and inside the loop you call a params span, then that would allocate each time, growing the stack untenably.

@CyrusNajmabadi
Copy link
Member

or just have a bunch of variables that are next to each other and create a span over them?

I missed this.

  1. This doesn't work as there's no concept of "next to each other" at all here :-)
  2. This is not how the runtime works. Imagine if you could just place 4 locals next to each other and pass a Span corresponding to a ref to the first element and a length of 4. Well, the runtime would have no reason to think anything what was special about the other 3 elements. It could trivially do stuff like GC them, thinking that they were no longer being referenced anywhere.

You need something the runtime actually understands as:

  1. Being actually contiguous
  2. Actually a construct that taking a ref to work keep everything in it alive.

@HaloFour
Copy link
Contributor

HaloFour commented Aug 6, 2023

@CyrusNajmabadi

We don't need to wait here at all. Compiler can generate whatever code is suitable depending on what the runtime and bcl make available. Initially, we'll use InlineArray (and we will synthesize the types if necessary). If a better option comes along later, the compiler can switch to it then.

Makes sense, was thinking that there may be observable differences. That's probably more true with inline arrays, but as long as the language supports both BufferX<T> and ValueArray<T, X> it's probably not an issue at all. With ValueArray<T, X> providing what appears to be a type that can be shared between assemblies perhaps the conversation around a simpler syntax could be reconsidered.

@CyrusNajmabadi
Copy link
Member

Yup yup!

@jaredpar
Copy link
Member

jaredpar commented Aug 7, 2023

I don't think const generics poses any particular issue here. How the array is passed is an implementation detail and one the spec specifically says can / will change at the discretion of the compiler / language. A conforming implementation could choose to heap allocate every array if it chose. The key is modeling params Span<T> as scoped so they are known to not escape which gives the compiler freedom in how the backing storage is allocated

Inline arrays are available today and fill the need for params Span<T> and as such will almost certainly be the initial way the feature is implemented. In the future if const generics come around, and have better characteristics, the compiler could flip to using those.

@MadsTorgersen
Copy link
Contributor

This proposal is now subsumed by the championed proposal #7700 for Params Collections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests