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

Use view buffers during pre-rendering #39465

Merged
merged 2 commits into from
Jan 18, 2022

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Jan 12, 2022

This change removes about 8kb of string[] allocations per request during pre-rendering and replaces them with a ViewBuffer that uses array pooling. The allocations come from list resizing as part of HtmlRenderer operations (such as https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/HtmlRenderer.cs#L133-L134). Also includes a couple of other clean up items:

  • Uses ValueTask instead of Task<T>
  • Moves top-level types to a separate file.
  • Some formatting cleanup

Details: https://github.com/dotnet/aspnetcore-internal/issues/3983

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 12, 2022
@pranavkm pranavkm force-pushed the prkrishn/view-buffers branch from 2bd50ea to 68d847e Compare January 12, 2022 20:45
@pranavkm pranavkm marked this pull request as ready for review January 13, 2022 18:20
@pranavkm pranavkm requested review from javiercn and a team as code owners January 13, 2022 18:20
Server,
WebAssembly,
ServerAndWebAssembly
var viewBuffer = new ViewBuffer(_viewBufferScope, nameof(ComponentRenderer), 16); // Preamble is fixed size
Copy link
Member

Choose a reason for hiding this comment

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

If we later change the preamble size, and don't realise we should also update this line, will we cause adverse perf effects? Is there any way to make it fail if we do that?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the instantiation of the viewbuffer should be done inside WebAssemblyComponentSerializer.AppendPreamble so it's all localized to one bit of code.

@SteveSandersonMS SteveSandersonMS self-requested a review January 18, 2022 12:14
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

This all looks good to me, even though the concept of MVC view buffers is new to me and so it's possible I might be missing some subtlety. I'm approving based on what I know, but would recommend getting confirmation from @javiercn too.

Copy link
Contributor Author

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

@SteveSandersonMS ViewBuffers are an MVC abstraction for ArrayPool<T> that are rented for the duration of view rendering. The view rendering system has some optimizations that makes it efficient to use these when rendering because we know it's safe to transfer ownership of the underlying arrays around since the rented lifetime is well understood:

@pranavkm pranavkm force-pushed the prkrishn/view-buffers branch from f8ce8ac to 70deb39 Compare January 18, 2022 18:48
@pranavkm pranavkm enabled auto-merge (squash) January 18, 2022 18:49
@pranavkm pranavkm merged commit 1ca0709 into dotnet:main Jan 18, 2022
@pranavkm pranavkm deleted the prkrishn/view-buffers branch January 18, 2022 20:57
@ghost ghost added this to the 7.0-preview1 milestone Jan 18, 2022
ShreyasJejurkar pushed a commit to ShreyasJejurkar/aspnetcore that referenced this pull request Jan 22, 2022
* Use view buffers during rendering

This change removes about 8kb of string[] allocations per request during pre-rendering and replaces them with a ViewBuffer that uses array pooling. The allocations come from list resizing as part of HtmlRenderer operations (such as https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.ViewFeatures/src/RazorComponents/HtmlRenderer.cs#L133-L134). Also includes a couple of other clean up items:

* Uses ValueTask instead of `Task<T>`
* Moves top-level types to a separate file.
* Some formatting cleanup
@davidfowl davidfowl added the Perf label Sep 30, 2022
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 Perf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants