Span Copy method that copies as much as possible

[jkotas]

From https://github.com/dotnet/corefx/issues/16840#issuecomment-285225905

Whenever use CopyTo on Span, I wish the method Always succeeded, copied as much as it can, and returned an int indicating how much was copied.
e.g.

int copied = 0;
while(copied < source.Length) {
    copied += source.Slice(copied).CopyTo(destination);
    destination = GetMoreSpace();
}

... and many similar scenarios. Without such CopyTo there is lots of ceremony around making sure the source is not larger than the destination.

@davidfowl, @terrajobst, @jkotas, any thoughts?

[jkotas]

I think it is a fine idea to have API like this, but I am not sure whether it should be called CopyTo - it would be different from the established CopyTo meaning.

[ahsonkhan]

@jkotas: Would we then remove the current CopyTo method then and have a new method that behaves as proposed here? What would happen to TryCopyTo?

@KrzysztofCwalina, @terrajobst, @shiftylogic: Have I summarized the sample usage of the current and proposed CopyTo methods correctly and sufficiently?

Here is the example of how Span.CopyTo would be used currently:

// source.Length > destination.Length - Throw ArgumentException("Destination is too short")
private static void SampleUsage1(Span<byte> source, Span<byte> destination)
{
    while (true)
    {
        try
        {
            source.CopyTo(destination);
            break;
        }
        catch (Exception e)
        {
            destination = GetMoreSpace(destination);
        }
    }
}

// source.Length > destination.Length - Throw ArgumentException("Destination is too short")
private static void SampleUsage2(Span<byte> source, Span<byte> destination)
{
    while (destination.Length < source.Length)
    {
        destination = GetMoreSpace(destination);
    }
    source.CopyTo(destination);
}

Here is the example of how Span.CopyTo would be used if it returns an int instead, as proposed:

// source.Length > destination.Length - Copy until destination is full, always succeed
private static void ProposedChangeSampleUsage(Span<byte> source, Span<byte> destination)
{
    int copied = source.CopyTo2(destination);
    while (copied < source.Length)
    {
        destination = GetMoreSpace(destination);
        copied += source.Slice(copied).CopyTo2(destination.Slice(copied));
    }
}

I am not sure whether it should be called CopyTo - it would be different from the established CopyTo meaning.

Any suggestions of a different name for the proposed method?

[jkotas]

Your SampleUsage1 is not how the code would be written today. try/catch on non-exception codepath is no-no.

You SampleUsage2 does not work. Is it missing something?

destination = GetMoreSpace(destination);

Is it really the case the GetModeSpace takes destination and gives you new one that has the old one copied into it? It would expect GetMoreSpace for this use case to give you a new buffer that starts from 0.

Any suggestions of a different name for the proposed method?

CopyAsMuchAsPossible

[ahsonkhan]

try/catch on non-exception codepath is no-no.

SampleUsage1 has try/catch around CopyTo which can (and does) throw an exception when source is larger than destination.

You SampleUsage2 does not work. Is it missing something?

I don't understand. I ran it in a test application and it works as expected. Why won't it work?

Is it really the case the GetModeSpace takes destination and gives you new one that has the old one copied into it? It would expect GetMoreSpace for this use case to give you a new buffer that starts from 0.

I did not fully understand the semantics of GetMoreSpace() from what was shown in the original post. I assumed it copied into a new span that is backed by larger array/memory. How else would you "enlarge" the span?

CopyAsMuchAsPossible

Would we then remove the current CopyTo method? What would happen to TryCopyTo?

[jkotas]

can (and does) throw an exception when source is larger than destination.

Right, it means that you are using exceptions for regular control flow - that's no-no.

I assumed it copied into a new span that is backed by larger array/memory

No. It would be unnecessary copy. It gives you a new buffer to copy what's remaining into.

Would we then remove the current CopyTo method?

The current CopyTo method matches what CopyTo methods do everywhere else in framework: https://github.com/dotnet/corefx/search?p=2&q=CopyTo&utf8=%E2%9C%93 . I do not see why we would want to remove it.

[davidfowl]

CopyAsMuchAsPossible

That's not a great name. TryCopyTo sounds better.

[KrzysztofCwalina]

@ahsonkhan, the "current" code sample is not the greatest:

1. As Jan said it cannot rely on exceptions
2. It's not really doing what you need to do. Here is a very long (and buggy) version that I wrote, as I did not have time to write a shorter one:

    private static void SampleUsage1(Span<byte> source, IOutput<byte> output)
    {
        Span<byte> destination = output.Buffer;
        if (destination.Length >= source.Length) {
            source.CopyTo(destination);
            return;
        }

        int toCopy = source.Length;
        int copied = 0;
        while (true) {
            var toCopyNow = Math.Min(toCopy - copied, destination.Length);
            var toCopyBytes = source.Slice(copied, toCopyNow);
            toCopyBytes.CopyTo(destination);
            copied += toCopyNow;
            if(copied < toCopyNow) {
                output.Resize();
                destination = output.Buffer;
            }
            else {
                return;
            }
        }
    }

And also, imagine what the code would look like if the source was multi-segmented too. :-)

[davidfowl]

Let's just add:

bool TryCopyTo<T>(Span<T> span, out int copied)

OR

void CopyTo<T>(Span<T> span, out int copied)

[KrzysztofCwalina]

When would TryCopyTo return false? If when destination is not large enough, then it's a bit confusing as the method would actually succesfully copy bytes.

CopyTo with out int parameter is better, but in some scenarios, it better to simply do:

copiedSoFar += segment.CopyTo(destination);

and now you will have to do:

segment.CopyTo(destination, out var justCopied);
copiedSoFar += justCopied;

This is not a blocking issues, but something to consider.

[jkotas]

Here is a very long (and buggy) version that I wrote, as I did not have time to write a shorter one:

I would be useful to write a version that actually works. Should it be this?

    private static void SampleUsage1(Span<byte> source, IOutput<byte> output)
    {
        while (true) {
            Span<byte> destination = output.Buffer;

            int available = destination.Length;
            if (available >= source.Length) {
                source.CopyTo(destination);
                output.Advance(source.Length);
                return;
            }

            source.Slice(0, available).CopyTo(destination);
            source = source.Slice(available);

            output.Advance(available);
        }
    }

[davidfowl]

I would be useful to write a version that actually works. Should it be this?

That's basically what this does https://github.com/dotnet/corefxlab/blob/041ec3aa8281a9203c5523dd875371519114ce6a/src/System.IO.Pipelines/WritableBufferExtensions.cs#L16. It really should be an extension on IOutput

[jkotas]

Perfect. How would the method be written if we had this API?

[ahsonkhan]

Any updates on this API proposal?

[jkotas]

We need a real example that actually works. The examples at the top are not very convincing.

This API was supposed to make this real method easier to implement, but I do not see how.

[ahsonkhan]

This API was supposed to make this real method easier to implement, but I do not see how.

@davidfowl, thoughts?

[ahsonkhan]

Difference between current method and one with the proposed CopyTo:
cc @KrzysztofCwalina, @davidfowl

Current: https://github.com/dotnet/corefxlab/blob/master/src/System.IO.Pipelines/WritableBufferExtensions.cs#L16

if (buffer.Buffer.IsEmpty)
{
    buffer.Ensure();
}

// Fast path, try copying to the available memory directly
if (source.Length <= buffer.Buffer.Length)
{
    source.CopyTo(buffer.Buffer.Span);
    buffer.Advance(source.Length);
    return;
}

var remaining = source.Length;
var offset = 0;

while (remaining > 0)
{
    var writable = Math.Min(remaining, buffer.Buffer.Length);

    buffer.Ensure(writable);

    if (writable == 0)
    {
        continue;
    }

    source.Slice(offset, writable).CopyTo(buffer.Buffer.Span);

    remaining -= writable;
    offset += writable;

    buffer.Advance(writable);
}

With CopyTo that returns an integer of how much it successfully copied:

if (buffer.Buffer.IsEmpty)
{
    buffer.Ensure();
}

int copied = source.CopyTo2(buffer.Buffer.Span);
buffer.Advance(copied);

var remaining = source.Length - copied;
while (remaining > 0)
{
    var writable = Math.Min(remaining, buffer.Buffer.Length);

    buffer.Ensure(writable);

    if (writable == 0)
    {
        continue;
    }

    copied += source.Slice(copied, writable).CopyTo2(buffer.Buffer.Span);
    remaining -= writable;

    buffer.Advance(writable);
}

[jkotas]

• This does not look like improvement to me. It makes it harder to see that the implementation is correct - the original was better.

copied += source.Slice(copied, writable).CopyTo2(buffer.Buffer.Span);
remaining -= writable;

• The old and new implementations do not have the exact same behavior
• Why does the implementation - both before and after - has the if (writeable == 0) { continue; } part? It does not look right that calling Ensure(0) in a loop would suddenly allocate more memory.

[ahsonkhan]

Why does the implementation - both before and after - has the if (writeable == 0) { continue; } part?

I don't know and I agree. I wasn't sure why we have that, but I suppose it isn't relevant to the issue at hand.

This does not look like improvement to me.

+1. @KrzysztofCwalina, thoughts? Did I incorrectly use the proposed API? @davidfowl

[davidfowl]

Why does the implementation - both before and after - has the if (writeable == 0) { continue; } part? It does not look right that calling Ensure(0) in a loop would suddenly allocate more memory.

The API isn't that intuitive. Ensure does make it grow 😄

[jkotas]

Opened dotnet/corefxlab#1383 about the non-intuitiveness.

[ahsonkhan]

Edit, in corefxlab, not corefx - i.e. dotnet/corefxlab#1383

[ahsonkhan]

@KrzysztofCwalina, @davidfowl, should we close this issue and keep CopyTo as is?

[davidfowl]

Yes, it's not critical right now.