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.3k stars 9.97k forks source link

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

Open ChristianWeyer opened 3 years ago

ChristianWeyer commented 3 years ago

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?

stefanloerwald commented 3 years ago

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 commented 3 years ago

@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.

/cc: @SteveSandersonMS am I missing something here?

ghost commented 3 years ago

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.

ghost commented 3 years ago

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 commented 2 years ago

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.

javiercn commented 2 years ago

@TanayParikh I believe Mackinnon wrote a prototype for this.

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

MackinnonBuck commented 2 years ago

Here are some ideas we had around September 2021 about a potential approach. These may be outdated by now, so we'll have to do some re-evaluation.

Blazor Code-Generated Parameter Setting

This document describes the future direction for code-generated parameter setting in Blazor. The current work can be found here. The rest of this document will be describing changes to the existing design.

Future Direction

Following is a summary of the design changes we've decided on:

On the topic of matching parameter names to parameters without the use of a dictionary, the idea we are considering at the time of this document's writing is building a trie of the parameter names during code-gen, then translating the trie to a series of if/else if statements that identify the parameter to set. Following is a rough example of what the generated code might look like:

[Parameter] public string MyParam1 { get; set; }
[Parameter] public bool MyArg2 { get; set; }
[Parameter] public int MyArg3 { get; set; }

/*
Trie of parameter names:
      M
      |
      y
     / \
    /   \
   P     A
   |     |
   a     r
   |     |
   r     g
   |    / \
   a   2   3
   |
   m
   |
   1
*/

bool TrySetValue(string parameterName, object value)
{
  if (parameterName.Length >= 6 && parameterName[0] == 'M' && parameterName[1] == 'y')
  {
    if (parameterName[2] == 'P')
    {
      if (parameterName.Length == 8 &&
          parameterName[3] == 'a' &&
          parameterName[4] == 'r' &&
          parameterName[5] == 'a' &&
          parameterName[6] == 'm' &&
          parameterName[7] == '1')
      {
        MyParam1 = (string)value;
        return true;
      }
    }
    else if (parameterName[2] == 'A')
    {
      if (parameterName.Length == 6 &&
          parameterName[3] == 'r' &&
          parameterName[4] == 'g')
      {
        if (parameterName[5] == '2')
        {
          MyArg2 = (bool)value;
          return true;
        }
        if (parameterName[5] == '3')
        {
          MyArg3 = (int)value;
          return true;
        }
      }
    }
  }

  return base.TrySetValue(parameterName, value);
}

It might also be possible to accomplish this translation by sorting the parameter names alphabetically and identifying the indices at which consecutive names differ, rather than using a trie.

SteveSandersonMS commented 2 years ago

This looks great to me.

It might also be possible to accomplish this translation by sorting the parameter names alphabetically and identifying the indices at which consecutive names differ, rather than using a trie.

Sounds interesting!

Or if going with the approach above, one super-minor tweak on the logic above would be to use the framework's string comparison logic on substrings where possible (without allocation), e.g.:

if (parameterName.Length == 8
    && parameterName.AsSpan(3).CompareTo("aram1", StringComparison.OrdinalIgnoreCase) == 0)

Don't know how deep we want to get into micro-benchmarking, but on the interpreter at least, there might be some win in using natively-implemented comparison logic instead of interpreting a char-by-char check. Also we'd need to decide whether to make the logic case-insensitive, which unfortunately I suspect we would need to for back-compat.

javiercn commented 2 years ago

We should take into account how routing does this and potentially apply the same approach here:

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 8 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.

egil commented 1 year ago

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.

ghost commented 1 year ago

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 commented 9 months ago

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.