Skip to content

Conversation

@BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Apr 10, 2021

Allocate a single StreamItemMessage per stream instead of per stream item

Unfortunately this does add a new API so will need to go to API review.

@BrennanConroy BrennanConroy added Perf area-signalr Includes: SignalR clients and servers api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Apr 10, 2021
@ghost
Copy link

ghost commented Apr 10, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@davidfowl
Copy link
Member

Split the changes so we can API review just the relevant one.

@BrennanConroy
Copy link
Member Author

d3ba6ab

@davidfowl
Copy link
Member

I meant so I can merge this PR

@BrennanConroy
Copy link
Member Author

lol, fine, I'll open a different PR.

@BrennanConroy BrennanConroy changed the title [SignalR] Avoid some per request allocations [SignalR] Allocate one StreamItemMessage per stream Apr 10, 2021
Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Would a derived internal type work instead of changing the public one? Item would need to be virtual.

@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Apr 12, 2021
@BrennanConroy BrennanConroy added this to the 6.0-preview4 milestone Apr 12, 2021
@BrennanConroy BrennanConroy merged commit 678e2a5 into main Apr 12, 2021
@BrennanConroy BrennanConroy deleted the brecon/streamalloc branch April 12, 2021 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-approved API was approved in API review, it can be implemented area-signalr Includes: SignalR clients and servers Perf

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants