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

Span: Add Span<T>.Fill(T value) #19550

Closed
nietras opened this issue Dec 4, 2016 · 15 comments
Closed

Span: Add Span<T>.Fill(T value) #19550

nietras opened this issue Dec 4, 2016 · 15 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@nietras
Copy link
Contributor

nietras commented Dec 4, 2016

As discussed briefly in https://github.com/dotnet/corefx/issues/13427 I propose to add a Fill(T value) method to Span<T> to make span the goto place for safe and fast memory operations.

This is analoguous to the same method approved for arrays in https://github.com/dotnet/corefx/issues/6695 and added in dotnet/coreclr#8137, dotnet/corert#2259 but still lacking optimization https://github.com/dotnet/coreclr/issues/8290, perhaps Array.Fill<T> could forward to Span<T>.Fill in the future...

Rationale and Usage

Provide a safe yet fast way of filling of any type of contiguous memory; managed or unmanaged.

var span = new Span<int>(array, 3, 10);
span.Fill(42);

The argumentation for adding this is similar to Array.Fill<T>, but here with a wider use case, since this can be used with both managed and unmanaged memory:

  • To increase the efficiency of code doing this and prevent people from reinventing the wheel.
  • Allow performance optimizations depending on memory type and contents.

Proposed API

Add a single method Fill(T value) to the existing Span<T> API:

public class Span<T>
{
     public void Fill(T value);
}

Open Questions

Open question is whether this should be added as a member method or an extension method. I would argue for a member method, since this will depend on internal representation, and would perhaps allow for the JIT intrinsic version to do optimizations that a generic extension method can't. If that is a valid argument?

@karelz @jkotas @omariom @benaadams @jamesqo

@davidfowl
Copy link
Member

Can we call it memset :trollface:

@benaadams
Copy link
Member

benaadams commented Dec 4, 2016

Also an offset+count method?

public class Span<T>
{
     public void Fill(T value);
     public void Fill(T value, int offset, int count);
}

Unless this pattern seems ok?

span.Slice(1, 12).Fill(42);

@jamesqo
Copy link
Contributor

jamesqo commented Dec 4, 2016

@benaadams I think having an offset+count one is redundant. Span is not like an array where you fundamentally cannot pass part of it around, so having that overload is just syntax sugar.

@benaadams
Copy link
Member

benaadams commented Dec 4, 2016

so having that overload is just syntax sugar

It is, if there is no slice cost, therefore no perf difference between

span.Slice(1, 12).Fill(42);

and

span.Fill(42, 1, 12);

They may end up being the same; e.g. Slice will bounds check; but Fill with extra params will also bounds check; Fill without extra params wouldn't have to bounds check.

If they both inlined, it would likely end up the same?

@jamesqo
Copy link
Contributor

jamesqo commented Dec 4, 2016

@benaadams 'syntax sugar' was not the correct term; I meant to say that it was redundant. The whole point of introducing Span was to avoid the offset/count pattern, wasn't it?

@jkotas
Copy link
Member

jkotas commented Dec 4, 2016

The whole point of introducing Span was to avoid the offset/count pattern, wasn't it?

Correct. You can tell from the existing Span surface, e.g. none of the Copy methods take offset/counts.

@benaadams
Copy link
Member

benaadams commented Dec 4, 2016

Just pointing out without offset/count its pushing a lot into inline Slicing.

Its a question of granularity and performance of Slice + Action.

So if I had two spans that represented a fixed size message packet and I wanted to copy a section from one to another the usage pattern would be:

spanSrc.Slice(4, 16).CopyTo(spanDst.Slice(4, 16));

Rather than

spanSrc.CopyTo(4, spanDst, 4, 16);

Which may be fine and end up with the same instructions being executed; or it may end up with extra items on the stack and extra checks being done.

@jamesqo
Copy link
Contributor

jamesqo commented Dec 4, 2016

@benaadams If it is less performant then that would probably be considered a problem with the JIT & should be fixed instead of being worked around, no?

@benaadams
Copy link
Member

should be fixed instead of being worked around, no?

Ideally, then it would hopefully also pick up similar patterns in other code.

@nietras
Copy link
Contributor Author

nietras commented Dec 5, 2016

span.Slice(1, 12).Fill(42);
spanSrc.Slice(4, 16).CopyTo(spanDst.Slice(4, 16));

Yes, I think that is the beauty of the Span/Slice API, we avoid combinatorial explosion in overloads so one only needs one version of each method for a given functionality.

E.g. CopyTo, Clear, Fill and any other method currently available for Array that I think should be made available for Span e.g. Sort, FindIndex... I think of Span<T> as a low level cousin of array that has the benefit of being useable on unmanaged memory. For example, how can I sort an unmanaged array in C# currently? There is no support for this. Span<T> unifies this. Not sure whether other people feel like that is the case or even ideal...

Just pointing out without offset/count its pushing a lot into inline Slicing.

I have the same concern here, but hope that this would be given attention if that was the case, and until this is proven a problem we should not design around it.

@karelz
Copy link
Member

karelz commented Dec 6, 2016

API review: Approved.

@karelz
Copy link
Member

karelz commented Dec 9, 2016

Note the issue is still up for grabs if anyone wants to take it.

It looks like it will be a little bit involved implementation (see attempt in dotnet/coreclr#8498)

@nietras
Copy link
Contributor Author

nietras commented Dec 9, 2016

@karelz I hope to give it a shot, but again am waiting for the Unsafe additions. Otherwise, it seems @jkotas is ok with a functional commit, before an optimized commit.

@karelz
Copy link
Member

karelz commented Dec 10, 2016

Thanks! Assigned to you.

@karelz karelz changed the title API Proposal: S.M.Span<T> add Fill(T value) Span: Add Span<T>.Fill(T value) Dec 18, 2016
@nietras
Copy link
Contributor Author

nietras commented Jan 15, 2017

Closed via dotnet/corefx#14396

@nietras nietras closed this as completed Jan 15, 2017
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

7 participants