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

Avoid buffering during input formatting for longer than necessary #9806

Merged
merged 4 commits into from
May 3, 2019

Conversation

pranavkm
Copy link
Contributor

EnableRewind uses FileBufferingReadStream which is not disposed until the response is completed.
This results in holding on to internal buffers for significantly longer than necessary. Changing it
to return the buffers immediately improved the allocations and throughput.

Description RPS CPU (%) Memory (MB) Avg. Latency (ms) Startup (ms) First Request (ms) Latency (ms) Ratio
Newtonsoft.Json - old - 500 bytes 49,661 98 2,349 7.32 514 87.97 0.49 1.00
Newtonsoft.Json - new - 500 bytes 120,273 94 174 2.32 509 80.54 0.36 2.42
Newtonsoft.Json - old - 2k 29,759 97 382 9.11 556 80.14 0.5 1.00
Newtonsoft.Json - new - 2k 36,079 97 186 7.53 507 84.96 0.41 1.21
Newtonsoft.Json - old - 8 kbytes 29,784 98 343 9.41 506 81.49 0.36 1.00
Newtonsoft.Json - new - 8 kbytes 35,408 98 187 8.79 503 79.64 0.33 1.19
Newtonsoft.Json - old - 40 kbytes 2,937 76 2,004 89.87 512 89.18 0.49 1.00
Newtonsoft.Json - new - 40 kbytes 8,416 99 202 30.8 534 78.79 0.47 2.87

Exception exception = null;
object model;

using (var streamReader = context.ReaderFactory(readStream, encoding))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just moved the ErrorHandler out of the try-catch block since it doesn't need to be there. Besides disposing the FileBufferingReadStream, the rest of this is unchanged.

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

What about the XML perf?

src/Http/WebUtilities/src/FileBufferingReadStream.cs Outdated Show resolved Hide resolved
Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Any particular thing you wanted me to look at @pranavkm?

@rynowak
Copy link
Member

rynowak commented Apr 28, 2019

So what the impact of this if you wanted to rewind and reread the stream? Would you see a non-seekable stream that's at the end?

What's required if a user wants to enforce buffering (and allow rewinding)?

I'm asking because I'm sure someone has done this, and we're changing the default.

@dotnet dotnet deleted a comment Apr 28, 2019
@Eilon Eilon added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-servers labels Apr 30, 2019
EnableRewind uses FileBufferingReadStream which is not disposed until the response is completed.
This results in holding on to internal buffers for significantly longer than necessary.Changing it
to  return the buffers immediately improved the allocations and throughput.
@pranavkm
Copy link
Contributor Author

pranavkm commented May 1, 2019

What's required if a user wants to enforce buffering (and allow rewinding)?

You can explicitly call EnableRewind in a middleware prior to getting to MVC and things should work as before. In fact, we avoid double buffering if we think the request stream is buffered.

@pranavkm
Copy link
Contributor Author

pranavkm commented May 1, 2019

🆙 📅

@pranavkm pranavkm merged commit 093df67 into master May 3, 2019
@ghost ghost deleted the prkrishn/quick-release branch May 3, 2019 13:45
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants