Add span-based CopyTo and CopyToAsync methods

[manandre]

Contributes to https://github.com/dotnet/corefx/issues/21472

[stephentoub]

Thanks for working on this.

A large part of the value of such APIs is derived types providing a better implementation. Can you please do so in this PR for the Streams in CoreLib, e.g.MemoryStream?

[stephentoub]

ps MemoryStream was one example. Others include NullStream, FileStream, UnmanagedMemoryStream, and probably another one or more I'm forgetting. Thanks.

[stephentoub]

Thanks for continuing to work on this, @manandre.

Still some open issues that looks like may have been missed:

• Why is the validation helper method necessary? Since the overload is just going to delegate to the Stream-based overload that already does these same checks, isn't the only check we need in this overload whether the callback is null?
• What about other streams in corelib, like NullStream, FileStream, UnmanagedMemoryStream, etc.?

Thanks.

[manandre]

Why is the validation helper method necessary? Since the overload is just going to delegate to the Stream-based overload that already does these same checks, isn't the only check we need in this overload whether the callback is null?

Can we safely rely on CopyTo/CopyToAsync(Stream) to ensure arguments validation? even if they are overriden by a custom stream implementation?
If yes, I agree to check only whether the callback is null.

What about other streams in corelib, like NullStream, FileStream, UnmanagedMemoryStream, etc.?

Let's discuss it:

• NullStream: The override only saves the creation of the WriteCallbackStream instance. Does it worth it?
• SyncStream (thread-safe wrapper): Nothing to do.
• FileStream: Only the Windows implementation overrides the CopyToAsync(Stream) method and I do not know how to optimize as it already uses ReadOnlyMemory<byte>. Any idea?
• MemoryStream: Done.
  • UnmanagedMemoryStreamWrapper: Do we apply the same pattern as CopyToAsync(Stream)?
• UnmanagedMemoryStream: Can we optimize the copy here?
  • PinnedBufferMemoryStream: Overload does not seem needed here.
  • ManifestResourceStream: Ditto

[maryamariyan]

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

1. In your coreclr repository clone, create patch by running git format-patch origin
2. In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>

[stephentoub]

Can we safely rely on CopyTo/CopyToAsync(Stream) to ensure arguments validation? even if they are overriden by a custom stream implementation?

Yes. What validation an override chooses to perform is up to that override, based on the logic the override is going to use in its implementation. When this overload delegates to it, it's up to the override to provide the validation needed for the arguments provided to it, and we just need to validate the others.

NullStream: The override only saves the creation of the WriteCallbackStream instance. Does it worth it?

Yes.

UnmanagedMemoryStream: Can we optimize the copy here?

For the sync overload, it can just create a span from its memory and call the action with that.

UnmanagedMemoryStreamWrapper
PinnedBufferMemoryStream
ManifestResourceStream

Don't worry about these; they're very specialized streams and it's unlikely this method is ever going to be used on them, and if it they happen to be, the overhead of using the base shouldn't be impactful.

FileStream

There's a lot of work happening in FileStream's existing CopyToAsync. Probably not worth overriding.

[manandre]

@stephentoub Can you please do a final review in order to give us a chance to get this pull request merged before the migration to dotnet/runtime? 🤞

[stephentoub]

Several remaining comments to be addressed. Otherwise, LGTM. Thanks.

[stephentoub]

/azp run coreclr-ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[stephentoub]

Thanks!

[manandre]

/azp run coreclr-ci

[azure-pipelines]

Commenter does not have sufficient privileges for PR 27639 in repo dotnet/coreclr

[manandre]

@stephentoub Help 🆘

[stephentoub]

Failures were unrelated.

Thanks!