Skip to content

Avoid repetitive GCHandles in IIS WebSocket operations #48462

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

Merged
merged 3 commits into from
Jun 8, 2023

Conversation

BrennanConroy
Copy link
Member

Make the AsyncIOEngine in IIS disposable so we can avoid creating a new GCHandle for every WebSocket read and write operation.

@Tratcher
Copy link
Member

What's the impact? Is it just allocations, or is there some perf cost to making the GC handle over and over?

@davidfowl
Copy link
Member

davidfowl commented May 27, 2023

There’s a performance cost yes. This is from an internal customer

@davidfowl
Copy link
Member

Do the uncached operations get disposed?

@BrennanConroy
Copy link
Member Author

Do the uncached operations get disposed?

It shouldn't be possible to get uncached ones right?
The read and write are in single loops
https://source.dot.net/#Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.IO.cs,105

@davidfowl
Copy link
Member

It shouldn't be possible to get uncached ones right?
The read and write are in single loops
https://source.dot.net/#Microsoft.AspNetCore.Server.IIS/Core/IISHttpContext.IO.cs,105

That's technically true, but the code isn't written that way.

@BrennanConroy
Copy link
Member Author

Would removing the Get*Operation() and ReturnOperation() methods make that clearer?

@davidfowl
Copy link
Member

I think so?

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
@BrennanConroy
Copy link
Member Author

Thoughts on new changes?

@davidfowl
Copy link
Member

This looks much easier to follow!

@BrennanConroy
Copy link
Member Author

@Tratcher Want to re-review? Or does your current approval still stand?

@BrennanConroy BrennanConroy merged commit d868553 into main Jun 8, 2023
@BrennanConroy BrennanConroy deleted the brecon/handleonce branch June 8, 2023 20:28
@ghost ghost added this to the 8.0-preview6 milestone Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-iis Includes: IIS, ANCM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants