Parameter name case for SetParametersAsync

[stefanloerwald]

Hi,

I came across performance best pactices by @SteveSandersonMS and @pranavkm and created a source generator to address the inefficiency of the default implementation of SetParametersAsync (see https://github.com/stefanloerwald/Generators.BetterBlazor).

It seems like route parameters (e.g. @ page "/{id}") are matching to parameters, even if the case isn't the same (so id seems to match Id). Is this by design? If so, is any case permitted?

Thanks
Stefan

[SteveSandersonMS]

Yes, parameter names are case-insensitive everywhere in Blazor. I know it would be nice for perf if they were actually case sensitive, but this is what we chose back in 3.0 and need to stick with it now.

I strongly recommend you don't call ToLowerInvariant(). That will destroy any perf gains you're making. It will have to make a new copy of the string entirely and walk across all the characters in it, and you will end up with a non-interned string.

Instead I'd do something like:

        private void BetterBlazorImplementation__WriteSingleParameter(string name, object value)
        {
            switch (name[0])
            {
                case "m":
                case "M":
                    if (string.Equals(name, nameof(MyFirstParameter), StringComparison.OrdinalIgnoreCase))
                    {
                        this.MyFirstParameter = (string)value;
                        return;
                    }
                    if (string.Equals(name, nameof(MySecondParameter), StringComparison.OrdinalIgnoreCase))
                    {
                        this.MySecondParameter = (string)value;
                        return;
                    }
                    break;
                case "y":
                case "Y":
                    if (string.Equals(name, nameof(YetAnotherParameter), StringComparison.OrdinalIgnoreCase))
                    {
                        this.YetAnotherParameter = (string)value;
                        return;
                    }
                    break;
            }
            throw new ArgumentException($"Unknown parameter: {name}");
        }

This retains the case-insensitivity, but reduces the number of string comparisons by branching on specific characters. Also, comparing against nameof(YetAnotherParameter) means that in the conventional usage with correct casing, you may well be comparing two references to the same interned string, which can be evaluated as true without walking through the characters.

If you want to be fancier still, you could completely reduce it to a single string comparison per param by branching on more characters, e.g.:

   switch (name[0])
   {
      case 'm':
      case 'M':
         switch (name[2])
         {
             case 'f':
             case 'F':
                 // test for MyFirstParameter or fail
             case 's':
             case 'S':
                 // test for MySecondParameter or fail
         }
         break;
   }

It just depends on how clever you want to make your generated code.

[SteveSandersonMS]

Finally, one other option: even through the framework itself treats parameter names as case-insensitive, it doesn't mean your library has to. Your generated code could choose to throw if the parameter can't be found case-sensitively. It just means that any developer using your solution has to provide correct casing, which they pretty much always do anyway because of intellisense.

[stefanloerwald]

Thanks @SteveSandersonMS for the reply. I'm not a fan of the ToLowerInvariant either, just wanted to make it correct as a first step. I measured the perf difference between a simple switch and a clever per-character nested switches and didn't see a significant performance improvement, but that was before I discovered the insensitivity. I think I'll replace it by a nested switch, first by name, then by name.ToLowerInvariant(), as that should give the expected performance for the hot path and still work with route parameters.

I also added a diagnostic for conflicting parameter names (e.g. Foo/FoO).

[stefanloerwald]

For those interested, the repo for the SG project moved to https://github.com/excubo-ag/Generators.Blazor

@SteveSandersonMS is this something you'd think should be moved to the framework at one point? And is there a plan to migrate the blazor compiler to roslyn-based source generators?

[SteveSandersonMS]

@stefanloerwald Yes, we'd certainly be interested in making this part of .NET 6. If you're interested in leading that effort, we'd need something like:

• Estimates of the perf differences in a few cases (e.g., simple component with only 1 parameter, complex component with 8 parameters, maybe others). This is to justify doing anything at all, and equally important, to understand the cases where it doesn't make much difference.
• A proposal for how this gets rolled out. Do we turn it on by default? Is there some switch? Per-project or per component? This needs to account for back-compatibility and aim to minimize the number of concepts we add to the system. Ideally there would be some totally safe way to turn it on in all cases for everyone, without any loss of back-compatibility.
• A final implementation with test cases.

Preferably in that order. There's no point producing a final implementation until the perf difference is justified and any APIs are agreed.

If you're interested in doing something like that, please let us know. If not, that's OK too - we may still do it ourselves but it depends on how other priorities are balanced.

And is there a plan to migrate the blazor compiler to roslyn-based source generators?

Long term we'd love to do that. We don't have a specific commitment to do it yet though. .NET 6 planning is still very much underway.

[stefanloerwald]

I'd be happy to take the lead on this.

Estimates of the perf differences in a few cases [...]

A primitive benchmark is already part of the project. I'll have to find the actual default implementation of SetParametersAsync in the framework, to perform a fair comparison (and also actually make the benchmark use inheritance, as that'll have a tiny overhead too).

A proposal for how this gets rolled out [...]

This is going to be tricky, as so far the SG relies on overriding the virtual method, hence the user cannot override as well (unless opting out of the SG, which is possible). So far, the easiest way to me seems an opt-in via attribute in _Imports.razor, with the optional opt-out-again with another attribute on specific components. It could simply be part of a "new project" template.

[SteveSandersonMS]

This is going to be tricky, as so far the SG relies on overriding the virtual method, hence the user cannot override as well

Consider options like changing ComponentBase to have:

protected virtual Task SetParametersAsync(ParameterView parameterView)
{
    WriteParametersToProperties(parameterView);
    ...
}

protected virtual void WriteParametersToProperties(ParameterView parameterView)
{
    // here goes the existing default reflection-based logic
    // also the Razor compiler can codegen an override of this new method if it wants
}

Now the developer can still override SetParametersAsync to replace or wrap the built-in logic, and whether or not we codegen an override of WriteParametersToProperties, it will still work.