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

lateapexearlyspeed-issue-30778 Create method: TryReadExact(). #57921

Conversation

lateapexearlyspeed
Copy link
Contributor

Fix #30778
Will change PR to normal status when it is ready for review, thanks.

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 23, 2021
@ghost
Copy link

ghost commented Aug 23, 2021

Tagging subscribers to this area: @GrabYourPitchforks, @dotnet/area-system-buffers
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #30778
Will change PR to normal status when it is ready for review, thanks.

Author: lateapexearlyspeed
Assignees: -
Labels:

area-System.Buffers, new-api-needs-documentation

Milestone: -

@lateapexearlyspeed lateapexearlyspeed marked this pull request as ready for review August 23, 2021 10:14
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me with 1 comment formatting suggestion. I'd like to get a secondary review from @tannergooding, @davidfowl, or @GrabYourPitchforks though.

@davidfowl
Copy link
Member

The implementation looks fine but I'm wondering if there's a more optimal implementation. Can we add benchmarks for this new method?

@lateapexearlyspeed
Copy link
Contributor Author

lateapexearlyspeed commented Oct 12, 2021

The implementation looks fine but I'm wondering if there's a more optimal implementation. Can we add benchmarks for this new method?

Hi @davidfowl Sorry I am not very familiar with workflow of adding benchmark. Because it is new method, is that dotnet runtime repo publishes new pack then perf repo references that new dotnet pack so that it can build to get new method ?

@jeffhandley
Copy link
Member

@lateapexearlyspeed The instructions are here: https://github.com/dotnet/performance/blob/main/docs/benchmarking-workflow-dotnet-runtime.md#benchmarking-workflow-for-dotnetruntime-repository

Let us know if you have any trouble getting that set up.

@davidfowl Any objections to merging this implementation as-is so long as we follow up with adding a new benchmark to dotnet/performance? Or do you want to see the (new, local) benchmark results before we merge?

@davidfowl
Copy link
Member

I'm fine merging it with the benchmark as a follow up

@jeffhandley
Copy link
Member

Thanks for the contribution, @lateapexearlyspeed! #60332 was filed for creating the microbenchmarks. If you're interested in contributing the new benchmarks, please leave a comment on that issue and I can assign it to you.

@jeffhandley jeffhandley merged commit 6890b50 into dotnet:main Oct 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Buffers community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API Proposal: SequenceReader<T>.TryRead overloads to read a specified number of elements
3 participants