search

dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.41k stars 10k forks source link

Blazor HtmlRenderer: Use ReadOnlySpan instead of ArrayRange #45259

Closed linkdotnet closed 1 year ago

linkdotnet commented 1 year ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe the problem.

This is not a feature request per se. The current HtmlRenderer is using ArrayRange as its collection type for passing around RenderTreeFrames.

Describe the solution you'd like

In bUnit, we have a mirror of that renderer in our code (more or less). We saw smaller performance gains in switching to ReadOnlySpan<RenderTreeFrame> as well as a slight improvement in readability. This would remove also code like ref var frame = ref frames.Array[position]. That's why I thought to push this change "upstream".

Additional context

The change from bUnit: https://github.com/bUnit-dev/bUnit/pull/708/commits/d65dbb183f25a54aaca25aa96733ee4c3b7468da

I can also provide a PR.

javiercn commented 1 year ago

@linkdotnet thanks for contacting us.

Do you happen to have some numbers collected for this? We are definitely happy about taking perf improvements provided that the numbers back up the claims :). We need to be very careful not to regress things here.

linkdotnet commented 1 year ago

@linkdotnet thanks for contacting us.

Do you happen to have some numbers collected for this? We are definitely happy about taking perf improvements provided that the numbers back up the claims :). We need to be very careful not to regress things here.

Let me (re)create a benchmark for that with .NET7 to drive the discussion a bit further.

ghost commented 1 year ago

Hi @linkdotnet. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

linkdotnet commented 1 year ago

Good input @javiercn . I specifically checked for smaller to medium-sized components and surprisingly ReadOnlySpan is slower in those scenarios (the smaller the more overhead ROS produces). That said ROS is not a suitable replacement. Therefore I'll close the ticket. If I do find other areas of improvement I will try to grab some numbers first.

Cheers Steven