Skip to content

[SignalR] Allocate one StreamItemMessage per stream - #31660

Merged
BrennanConroy merged 1 commit into
mainfrom
brecon/streamalloc
Apr 12, 2021
Merged

[SignalR] Allocate one StreamItemMessage per stream#31660
BrennanConroy merged 1 commit into
mainfrom
brecon/streamalloc

Conversation

@BrennanConroy

@BrennanConroy BrennanConroy commented Apr 10, 2021

Copy link
Copy Markdown
Member

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

ghost commented Apr 10, 2021

Copy link
Copy Markdown

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
Copy Markdown
Member

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

@BrennanConroy

Copy link
Copy Markdown
Member Author
@davidfowl

Copy link
Copy Markdown
Member

I meant so I can merge this PR

@BrennanConroy

Copy link
Copy Markdown
Member Author

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

@BrennanConroy BrennanConroy changed the title [SignalR] Avoid some per request allocations Apr 10, 2021

@davidfowl davidfowl left a comment

Copy link
Copy Markdown
Member

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

3 participants