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

Consider using C#9 Source Generators for implementing SetParametersAsync #29550

Open
ChristianWeyer opened this issue Jan 22, 2021 · 13 comments
Open
Labels
affected-most This issue impacts most of the customers area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-blazor-component-model Any feature that affects the component model for Blazor (Parameters, Rendering, Lifecycle, etc) feature-rendering Features dealing with how blazor renders components Pillar: Technical Debt Priority:1 Work that is critical for the release, but we could probably ship without severity-minor This label is used by an internal tool

Comments

@ChristianWeyer
Copy link

The performance best practices here talk about a custom implementation of SetParametersAsync instead of using reflection:
https://docs.microsoft.com/en-us/aspnet/core/blazor/webassembly-performance-best-practices?view=aspnetcore-5.0#implement-setparametersasync-manually

@stefanloerwald has built a lib that uses C#9 source generators that shows significant perf improvements:
https://github.com/excubo-ag/Generators.Blazor

Could that be an approach to include into the Blazor framework?

@ChristianWeyer ChristianWeyer added the design-proposal This issue represents a design proposal for a different issue, linked in the description label Jan 22, 2021
@Pilchie Pilchie added the area-blazor Includes: Blazor, Razor Components label Jan 22, 2021
@stefanloerwald
Copy link

Thanks for the flowers @ChristianWeyer. Just for the record: the perf improvement I measured was in a very narrow benchmark, which doesn't fully reflect the performance improvement in the context of the entire blazor framework.
That said, I think a lot can be done with source generators. Given that there is the idea out there to rewrite the razor compiler using source generators, there's a good opportunity to integrate such things by default.

@javiercn
Copy link
Member

@ChristianWeyer thanks for contacting us.

We looked at this as part of 5.0. It does improve perf a bit (@SteveSandersonMS has the data) but we didn't see any significant improvement and there were a few things we were concerned about, which is why we didn't do it.

  • It comes at the risk of introducing behavior changes over the stock implementation.
  • It introduces servicing issues (what happens if there's a security issue in the generated code).
  • We didn't have a way in the Razor compiler to do this in a reasonable way. (Since we couldn't know when a component was overriding SetParametersAsync in component base).

/cc: @SteveSandersonMS am I missing something here?

@mkArtakMSFT mkArtakMSFT removed the design-proposal This issue represents a design proposal for a different issue, linked in the description label Jan 25, 2021
@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Jan 25, 2021
@mkArtakMSFT mkArtakMSFT added this to the Next sprint planning milestone Jan 25, 2021
@ghost
Copy link

ghost commented Jan 25, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@SteveSandersonMS SteveSandersonMS added affected-most This issue impacts most of the customers severity-minor This label is used by an internal tool labels Jan 26, 2021 — with ASP.NET Core Issue Ranking
@javiercn javiercn added feature-blazor-component-model Any feature that affects the component model for Blazor (Parameters, Rendering, Lifecycle, etc) feature-rendering Features dealing with how blazor renders components labels Apr 20, 2021
@ghost
Copy link

ghost commented Jul 20, 2021

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@TanayParikh
Copy link
Contributor

We looked at this as part of 5.0. It does improve perf a bit (@SteveSandersonMS has the data) but we didn't see any significant improvement and there were a few things we were concerned about, which is why we didn't do it.

  • It comes at the risk of introducing behavior changes over the stock implementation.
  • It introduces servicing issues (what happens if there's a security issue in the generated code).
  • We didn't have a way in the Razor compiler to do this in a reasonable way. (Since we couldn't know when a component was overriding SetParametersAsync in component base).

Thanks Javier for the details. Closing this as answered.

@TanayParikh TanayParikh added the ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. label Oct 19, 2021
@ghost ghost added the Status: Resolved label Oct 19, 2021
@javiercn javiercn reopened this Oct 19, 2021
@javiercn
Copy link
Member

@TanayParikh I believe Mackinnon wrote a prototype for this.

We might want to re-open this topic, so leaving it open.

@TanayParikh TanayParikh removed ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved labels Oct 19, 2021
@TanayParikh TanayParikh added the Priority:2 Work that is important, but not critical for the release label Oct 22, 2021
@mkArtakMSFT mkArtakMSFT removed the Priority:2 Work that is important, but not critical for the release label Nov 2, 2021
@mkArtakMSFT mkArtakMSFT added Priority:1 Work that is critical for the release, but we could probably ship without and removed Priority:1 Work that is critical for the release, but we could probably ship without labels Nov 10, 2022
@mkArtakMSFT mkArtakMSFT removed the ss-p1 label Nov 22, 2022
@egil
Copy link
Contributor

egil commented Feb 5, 2023

I like the idea of optimizing parameter setting. No reason not to lean into the fact that C# is a statically typed language.

In bUnit, we are able to stub/mock components during testing, due to the fact that parameters are assigned in a "duck typing" fashion at runtime. Essentially, we are able to replace one component with another (type), as long as it has the same parameters as the original, or a "capture all unmatched parameters" parameter.

If I read the ideas in this issue correctly, that should still work, i.e. the basis is still that a component has a SetParameterAsync method that will still be called by the renderer with a ParameterView, but a source generator may generate an optimized overridden version of said method.

@mkArtakMSFT mkArtakMSFT modified the milestones: .NET 8 Planning, Backlog Mar 13, 2023
@ghost
Copy link

ghost commented Mar 13, 2023

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@ghost
Copy link

ghost commented Dec 12, 2023

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-most This issue impacts most of the customers area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-blazor-component-model Any feature that affects the component model for Blazor (Parameters, Rendering, Lifecycle, etc) feature-rendering Features dealing with how blazor renders components Pillar: Technical Debt Priority:1 Work that is critical for the release, but we could probably ship without severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

10 participants