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

Add overload of ByteString.CopyTo that can copy a slice #6230

Conversation

erikmav
Copy link
Contributor

@erikmav erikmav commented Jun 7, 2019

No description provided.

@jtattermusch
Copy link
Contributor

What is the use case?
Btw, if you are on netstandard2.0 (which has been around for a while now), you don't need this API. You can use ByteString.Span method to gain readonly access to byteString's data directly (which is more efficient and flexible than the API you're proposing).

public ReadOnlySpan<byte> Span => new ReadOnlySpan<byte>(bytes);

@erikmav
Copy link
Contributor Author

erikmav commented Jun 11, 2019

I'll move toward that over time. In this case I'm working in a net472 codebase, the .Span property is not visible in this case, so I was optimizing for what was available to minimize copies.

@erikmav erikmav closed this Jun 11, 2019
@erikmav erikmav reopened this Jun 14, 2019
@erikmav
Copy link
Contributor Author

erikmav commented Jun 14, 2019

@jtattermusch Memory only supported on netcore2.1+. I'd prefer to get this change in for downlevel net472 optimization.

@jtattermusch
Copy link
Contributor

the point about supporting optimizations for net472 is fair. I think we could add CopyFrom(ReadOnlySpan<byte> bytes) (see #5835) to the net45 target too, which would make a cleaner solution than what this PR proposes.

@jskeet @mgravell @JamesNK WDYT?

@mgravell
Copy link

I guess it comes down that "unity" discussion again; i.e. if "unity" is a supported target, what TFM does "unity" consume? if it is netstandard1.*, then: great - no harm, no foul; if it is net45 then it gets into that awkward spot of

  1. is there an API that unity can't exploit - which to be clear isn't a problem - there are other APIs, but more importantly:
  2. does the generated code now use that API by default, making it impossible to compile on the down-level unity compiler

@jskeet
Copy link
Contributor

jskeet commented Jun 28, 2019

Marc's given a better response than I would have done, basically :)
(And I suspect the answer to the question of Unity TFMs is going to be really complex, based on the version of Unity and whatever target the app is being built for.)

@jtattermusch
Copy link
Contributor

The current version of unity supports both net45 and netstandard2.0, so both TFM targets can be consumed by unity users and it depends on them what they choose.

  • the generated code currently doesn't use any API that would require require Span<> so from that respect there should be no problem on Unity

@erikmav
Copy link
Contributor Author

erikmav commented Aug 13, 2019

@jtattermusch Can you clarify the result of the thread above? Do I need to add #if for a net45 target, or is this PR acceptable as-is?

@erikmav
Copy link
Contributor Author

erikmav commented Aug 14, 2019

Closing in deference to a possible re-do as expansion of #5835 to a net45 target.

@erikmav erikmav closed this Aug 14, 2019
@jtattermusch
Copy link
Contributor

I think the plan is to make Google.Protobuf depend on System.Memory for all targets (see #6317 which is currently blocked on some nugets being released as stable) and with that everyone will be able to access the data with byteString.Span property (without needing an additional CopyTo method).

@erikmav erikmav deleted the dev/erikmav/byteStringCopyToSlice branch May 19, 2022 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants