Skip to content

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented Oct 7, 2022

Updates the runtime helper APIs the compiler will use with bind:get, bind:set, and bind:after.

Description

There is a full technical description of the issue all the way down the issue for all the details.

The TL;DR is that due to the way we decided to implement this feature, customers can run into situations where their components render many more times than expected, which results in confusion and performance degradation.

This was due to the fact that we chose to rely on EventCallback to implement this feature and we did not realize there were some side effects associated with it.

The APIs we are adding bypass the whole EventCallback infrastructure (simplifying the implementation too) and fix the issue.

Fixes #43195

Customer Impact

Low performance, unexpected behavior.

The reason it is important to fix this issue is because it is the result of feedback from the previews and since it involves the razor compiler, we won't be able to easily fix it in the future without introducing a breaking behavior change.

Regression?

  • Yes
  • No

[If yes, specify the version the behavior has regressed from]

Risk

  • High
  • Medium
  • Low

The only reason this is medium and not low is because it has a sibling PR on the razor-compiler to update it to use the new methods.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

Complete description and detailed technical analisys

bind get, set, after changes

There was an oversight on the initial design of bind get, set, after. We decided to piggyback on the EventCallback infrastructure to simplify the implementation as the runtime helpers that we already have provide support for coercing functions and actions into an event callback (typed or untyped).

Unfortunately, that has an additional unintended side-effect. The runtime helpers that the compiler uses for creating EventCallback have logic in them to select a "receiver" for the callback. If the callback target implements IHandleEvent, it gets chosen as the receiver. Otherwise, the explicit receiver passed in is used.

The issue is that when the receiver implements IHandleEvent, the EventCallback infrastrucure will call receiver.HandleEventAsync(evt). In the most common implementation (ComponentBase) this will in turn call StateHasChanged() internally up to two times (at the end of the sync and async work).

The result of this is that when using bind get, set, after StateHasChanged will be called more than it is needed, which surprises users and can cause unintended performance problems and other side effects.

The solution for this unfortunately, involves updating the runtime helper APIs for this scenario as well as the compiler to generate new code that consumes these new APIs.

We can piggyback on EventCallback APIs, but since we need to author new APIs, it is best to avoid this altogether.

Before we go into details on the concrete APIs we will recap the list of supported scenarios.

Binding to elements

In this case we are binding to a DOM event.

<input @bind="_val" />
private string _val = "";

Generated callback code

factory.CreateBinder(this, __value => _val == __value, _val)

CreateBinder has overloads for the different primitive types we support binding and each overload can receive either an Action or an EventCallback that was added as part of the support we did for bind get, set, after.

Binding to components

In this case we have a "parent" component and a "child" component where the "parent" component is binding to a pair of properties in the child component (for example Value and ValueChanged). ValueChanged can have two different types: Action<T>, EventCallback<T>.

Child component

[Parameter] public string Value { get; set; }

[Parameter] public EventCallback<string> ValueChanged { get; set; }

Parent component

<Child @bind-Value="_value" />

Generated code (EventCallback)

EventCallback.Factory.Create<string>(this, RuntimeHelpers.CreateInferredEventCallback(this, __value => _myVal = __value, _myVal));

In this case, we use a helper CreateInferredEventCallback to convert the function to the right type of event callback, since writing the generic signature is complicated when we have generic type arguments.

Generated code (Action)

(Action<string>)(__value => _myVal = __value)

In this case, we emit a type cast.

Bind to elements using set or after

In this scenario you are allowed to provide a callback which can be an Action<T>, a Func<T, Task> or an EventCallback<T> for bind:set or an Action, a Func<Task> or an EventCallback for bind:after.

Bind to elements using bind:set

<input @bind:get="_val" @bind:set="(value) => {_val = value; DoSomething(); }" />

In this scenario we update the generated code to create a setter ourselves, and since we can't know if what the user provides is a function, an action or an explicit EventCallback, we use the CreateInferredEventCallback overloads to coerce everything into an EventCallback<T>.

factory.CreateBinder(
  this,
  RuntimeHelpers.CreateInferredEventCallback(
    this,
    (value) => {_val = value; DoSomething(); },
    _val),
  _val);

Bind to elements using bind:after

<input @bind:get="_val" @bind:after="() => { DoSomething(); }" />

In this scenario things are even more complicated as we need to compose a setter that invokes the after function.

factory.CreateBinder(
  this,
  RuntimeHelpers.CreateInferredEventCallback(
    this,
    (__value) => { 
        _val = __value;
        return EventCallback.Factory.Create(() => { DoSomething(); }).InvokeAsync();
    },
    _val),
  _val);

The issue then becomes that the event callback at EventCallback.Factory.Create(() => { DoSomething(); }) as well as the RuntimeHelpers.CreateInferredEventCallback that wraps it, do more work than they should do. (As described above, they don't just invoke the callback, they'll call IHandleEvent.HandleEvent on the receiver if present)

As a result, we want to tweak the generated code as follows. Instead of using CreateInferredEventCallback, we are going to use a new helper CreateInferredBindSetter with the following signature and two overloads:

  • Func<TValue, Task> CreateInferredBindSetter<TValue>(Action<TValue> setter, TValue value)
  • Func<TValue, Task> CreateInferredBindSetter<TValue>(Func<TValue, Task> setter, TValue value)

We use these two overloads because we do not know when we receive a setter expression whether it is a Task returning function or not.

We will remove the EventCallback based runtime APIs that we added as part of the feature as we do not need them.

We will avoid using CreateEventCallback for BindAfter as we already have InvokeAsynchronousDelegate to coalesce bind:after expressions and invoke them.

Generated code before

Element bind get, set

<input type="text" @bind:get="_value" @bind:set="(value) => _value = value" />
__builder.AddAttribute(2, "value", global::Microsoft.AspNetCore.Components.BindConverter.FormatValue(
      _value
));
__builder.AddAttribute(
  3,
  "onchange",
  global::Microsoft.AspNetCore.Components.EventCallback.Factory.CreateBinder(
    this,
    global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.CreateInferredEventCallback(
      this, 
      callback: (value) => _value = value,
      value: _value),
      _value));

__builder.SetUpdatesAttributeName("value");

Element bind after

<input type="text" @bind="_value" @bind:after="() => {}" />
__builder.AddAttribute(7, "value", global::Microsoft.AspNetCore.Components.BindConverter.FormatValue(
        _value
));

__builder.AddAttribute(
  8,
  "onchange",
  global::Microsoft.AspNetCore.Components.EventCallback.Factory.CreateBinder(
      this,
      global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.CreateInferredEventCallback(
          this,
          callback: __value => { 
              _value = __value;
              return global::Microsoft.AspNetCore.Components.EventCallback.Factory.Create(
                  this,
                  callback: () => {}).InvokeAsync();
              },
          value: _value),
      _value));
__builder.SetUpdatesAttributeName("value");

Component bind after event callback

Child component

@code {
    public string Value { get; set; } = "";
    public EventCallback<string> ValueChanged { get; set; }
}

Parent component

<ChildComponent @bind-Value="Value" @bind-Value:after="() => { }" />

Generated code

__builder.AddAttribute(1, "Value", 
     Value
);
__builder.AddAttribute(
  2,
  "ValueChanged",
  global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.CreateInferredEventCallback(
      this,
      global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.CreateInferredEventCallback(
        this,
        callback: __value => {
            Value = __value;
            return global::Microsoft.AspNetCore.Components.EventCallback.Factory.Create(
                this,
                callback: () => { }).InvokeAsync(); 
            },
        value: Value),
        Value));

Component bind get, set event callback

Child component

@code {
    public string Value { get; set; } = "";
    public EventCallback<string> ValueChanged { get; set; }
}

Parent component

<ChildComponent @bind-Value:get="Value" @bind-Value:set="value => Value = value" />

Generated code

__builder.AddAttribute(1, "Value", 
    Value
);
__builder.AddAttribute(
  2,
  "ValueChanged",
  global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.CreateInferredEventCallback(
      this,
      value => Value = value,
      Value));
__builder.CloseComponent();

Component bind after Task returning function

Child component

@code {
    [Parameter] public string Value { get; set; } = "";
    [Parameter] public Func<string, Task> ValueChanged { get; set; }
}

Parent component

<ChildComponent @bind-Value="Value" @bind-Value:after="() => { }" />

Generated code

__builder.AddAttribute(1, "Value", global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.TypeCheck<global::System.String>(
    Value
));
__builder.AddAttribute(
    2,
    "ValueChanged",
    (global::System.Func<System.String, System.Threading.Tasks.Task>)(async __value => {
        Value = __value;
        await global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.InvokeAsynchronousDelegate(() => { });
    }));

Component bind get, set async returning function

Child component

@code {
    [Parameter] public string Value { get; set; } = "";
    [Parameter] public Func<string, Task> ValueChanged { get; set; }
}

Parent component

<ChildComponent @bind-Value:get="Value" @bind-Value:set="value => Value = value" />

Generated code

__builder.AddAttribute(1, "Value", global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.TypeCheck<global::System.String>(
    Value
));
__builder.AddAttribute(
    2,
    "ValueChanged",
    (global::System.Func<System.String, System.Threading.Tasks.Task>)((value) => { return Task.CompletedTask; }));

Component bind after action

Child component

@code {
    [Parameter] public string Value { get; set; } = "";
    [Parameter] public Action<string> ValueChanged { get; set; }
}

Parent component

<ChildComponent @bind-Value="Value" @bind-Value:after="() => { }" />

Generated code

__builder.AddAttribute(1, "Value", global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.TypeCheck<global::System.String>(
    Value
));
__builder.AddAttribute(
    2,
    "ValueChanged",
    (global::System.Action<System.String>)(__value => {
        Value = __value;
        global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.InvokeSynchronousDelegate(() => { });
    }));

Component bind get, set async returning function

Child component

@code {
    [Parameter] public string Value { get; set; } = "";
    [Parameter] public Action<string> ValueChanged { get; set; }
}

Parent component

<ChildComponent @bind-Value:get="Value" @bind-Value:set="value => Value = value" />

Generated code

__builder.AddAttribute(1, "Value", global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.TypeCheck<global::System.String>(
    Value
));
__builder.AddAttribute(
    2,
    "ValueChanged",
    (global::System.Action<System.String>)((value) => { Value = value; }));

Generated code after

Element bind get, set

<input type="text" @bind:get="_value" @bind:set="(value) => _value = value" />
__builder.AddAttribute(2, "value", global::Microsoft.AspNetCore.Components.BindConverter.FormatValue(
      _value
));
__builder.AddAttribute(
  3,
  "onchange",
  global::Microsoft.AspNetCore.Components.EventCallback.Factory.CreateBinder(
    this,
    global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.CreateInferredBindSetter(
      callback: (value) => _value = value,
      value: _value),
      _value));

__builder.SetUpdatesAttributeName("value");

Element bind after

<input type="text" @bind="_value" @bind:after="() => {}" />
__builder.AddAttribute(7, "value", global::Microsoft.AspNetCore.Components.BindConverter.FormatValue(
        _value
));

__builder.AddAttribute(
  8,
  "onchange",
  global::Microsoft.AspNetCore.Components.EventCallback.Factory.CreateBinder(
      this,
      global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.CreateInferredBindSetter(
          callback: __value => { 
              _value = __value;
              return InvokeAsynchronousDelegate(() => { });
          },
          _value),
      _value);
__builder.SetUpdatesAttributeName("value");

Component bind after event callback

Child component

@code {
    public string Value { get; set; } = "";
    public EventCallback<string> ValueChanged { get; set; }
}

Parent component

<ChildComponent @bind-Value="Value" @bind-Value:after="() => { }" />

Generated code

__builder.AddAttribute(1, "Value", 
     Value
);
__builder.AddAttribute(
  2,
  "ValueChanged",
  global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.CreateInferredEventCallback(
      this,
      global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.CreateInferredBindSetter(
        callback: __value => {
            Value = __value;
            return InvokeAsynchronousDelegate(() => { }); 
            },
        value: Value),
      Value));

Component bind get, set event callback

Child component

@code {
    public string Value { get; set; } = "";
    public EventCallback<string> ValueChanged { get; set; }
}

Parent component

<ChildComponent @bind-Value:get="Value" @bind-Value:set="value => Value = value" />

Generated code

__builder.AddAttribute(1, "Value", 
    Value
);
__builder.AddAttribute(
  2,
  "ValueChanged",
  global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.CreateInferredBindSetter(
      this,
      value => Value = value,
      Value));
__builder.CloseComponent();

Component bind after Task returning function

Child component

@code {
    [Parameter] public string Value { get; set; } = "";
    [Parameter] public Func<string, Task> ValueChanged { get; set; }
}

Parent component

<ChildComponent @bind-Value="Value" @bind-Value:after="() => { }" />

Generated code

__builder.AddAttribute(1, "Value", global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.TypeCheck<global::System.String>(
    Value
));
__builder.AddAttribute(
    2,
    "ValueChanged",
    (global::System.Func<System.String, System.Threading.Tasks.Task>)(async __value => {
        Value = __value;
        await global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.InvokeAsynchronousDelegate(() => { });
    }));

Component bind get, set async returning function

Child component

@code {
    [Parameter] public string Value { get; set; } = "";
    [Parameter] public Func<string, Task> ValueChanged { get; set; }
}

Parent component

<ChildComponent @bind-Value:get="Value" @bind-Value:set="value => Value = value" />

Generated code

__builder.AddAttribute(1, "Value", global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.TypeCheck<global::System.String>(
    Value
));
__builder.AddAttribute(
    2,
    "ValueChanged",
    (global::System.Func<System.String, System.Threading.Tasks.Task>)((value) => { return Task.CompletedTask; }));

Component bind after action

Child component

@code {
    [Parameter] public string Value { get; set; } = "";
    [Parameter] public Action<string> ValueChanged { get; set; }
}

Parent component

<ChildComponent @bind-Value="Value" @bind-Value:after="() => { }" />

Generated code

__builder.AddAttribute(1, "Value", global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.TypeCheck<global::System.String>(
    Value
));
__builder.AddAttribute(
    2,
    "ValueChanged",
    (global::System.Action<System.String>)(__value => {
        Value = __value;
        global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.InvokeSynchronousDelegate(() => { });
    }));

Component bind get, set async returning function

Child component

@code {
    [Parameter] public string Value { get; set; } = "";
    [Parameter] public Action<string> ValueChanged { get; set; }
}

Parent component

<ChildComponent @bind-Value:get="Value" @bind-Value:set="value => Value = value" />

Generated code

__builder.AddAttribute(1, "Value", global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.TypeCheck<global::System.String>(
    Value
));
__builder.AddAttribute(
    2,
    "ValueChanged",
    (global::System.Action<System.String>)((value) => { Value = value; }));

@javiercn javiercn requested a review from a team as a code owner October 7, 2022 17:29
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Oct 7, 2022
@javiercn javiercn added ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. Servicing-consider Shiproom approval is required for the issue and removed ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. labels Oct 7, 2022
@ghost
Copy link

ghost commented Oct 7, 2022

Hi @javiercn. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@mkArtakMSFT mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Oct 7, 2022
@dougbu dougbu merged commit 0dacc32 into release/7.0 Oct 11, 2022
@dougbu dougbu deleted the javiercn/bind-after-render branch October 11, 2022 19:47
Copy link
Contributor

@TanayParikh TanayParikh left a comment

Choose a reason for hiding this comment

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

:shipit: 🎉

@dougbu dougbu added this to the 7.0.0 milestone Nov 8, 2022
javiercn added a commit that referenced this pull request Jan 12, 2023
* Runtime helpers update
* Test updates
* Fix build break
wtgodbe pushed a commit that referenced this pull request Jan 12, 2023
* Runtime helpers update
* Test updates
* Fix build break
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants