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/ReadonlySpan TryCopy/CopyTo are very slow #16840

Closed
davidfowl opened this issue Mar 8, 2017 · 7 comments
Closed

Span/ReadonlySpan TryCopy/CopyTo are very slow #16840

davidfowl opened this issue Mar 8, 2017 · 7 comments

Comments

@davidfowl
Copy link
Contributor

@davidfowl davidfowl commented Mar 8, 2017

The implementation of TryCopyTo/CopyTo is extremely slow (byte by byte) for the portable span. There's a TODO to use a new API in Unsafe (which has since been added) but it was never implemented.

// TODO: This is a tide-over implementation as we plan to add a overlap-safe cpblk-based api to Unsafe. (https://github.com/dotnet/corefx/issues/13427)

// TODO: This is a tide-over implementation as we plan to add a overlap-safe cpblk-based api to Unsafe. (https://github.com/dotnet/corefx/issues/13427)

/cc @jkotas @benaadams @ahsonkhan @KrzysztofCwalina @nietras

@KrzysztofCwalina

This comment has been minimized.

Copy link
Member

@KrzysztofCwalina KrzysztofCwalina commented Mar 9, 2017

@ahsonkhan is working on this.

@KrzysztofCwalina

This comment has been minimized.

Copy link
Member

@KrzysztofCwalina KrzysztofCwalina commented Mar 9, 2017

@ahsonkhan, while you are doing th eperf work. I would also like us to consider changing the signature of CopyTo. 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

This comment has been minimized.

Copy link
Member

@jkotas jkotas commented Mar 9, 2017

We should not be hijacking this issue with orthogonal API design discussion. Opened #16888

@shiftylogic

This comment has been minimized.

Copy link
Contributor

@shiftylogic shiftylogic commented Mar 15, 2017

Fixed by #17063

@davidfowl davidfowl reopened this Mar 19, 2017
@davidfowl

This comment has been minimized.

Copy link
Contributor Author

@davidfowl davidfowl commented Mar 19, 2017

@shiftylogic This needs to be implemented for ReadOnlySpan<T>

@ahsonkhan

This comment has been minimized.

Copy link
Member

@ahsonkhan ahsonkhan commented Mar 21, 2017

Fixed in #17297

@davidfowl

This comment has been minimized.

Copy link
Contributor Author

@davidfowl davidfowl commented Mar 21, 2017

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.