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.37k stars 9.99k forks source link

Optimize RenderBatchWriter for empty diffs #45831

Open petr-horny-bsshop opened 1 year ago

petr-horny-bsshop commented 1 year ago

Is there an existing issue for this?

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

If we have a component that creates an empty diff when re-rendering, i.e. there was no change in the resulting markup, this empty diff is still sent to the server and takes up a certain size in the serialized data.

For example, consider this component:

@Value.DayOfWeek

@code {

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

Render the above component 1000 times:

@page "/"
<button @onclick="@(StateHasChanged)">Render</button>
<br />

@for (var i = 0; i < 1000; i++)
{
    <Component Value="DateTime.Now" />
}

Click the "Render" button.

Despite the fact that the resulting markup does not change (it changes once every 24 hours), 12 kB of data is sent to the server:

2023-01-02 14_51_54-PlayGround (Debugging) - Microsoft Visual Studio

2023-01-02 14_52_27-+  0  = {Microsoft AspNetCore Components RenderTree RenderTreeDiff}

It can be seen that the data sent is very "sparse":

2023-01-02 14_49_34-BlazorServerApp1

This particular case could of course be optimized using the ShouldRender method. However, in general it doesn't make sense (at least I think so) to send an empty diff to the client.

Describe the solution you'd like

Modify the int Write(in ArrayRange<RenderTreeDiff> diffs) method in RenderBatchWriter to ignore empty diffs.

Additional context

No response

mkArtakMSFT commented 1 year ago

Thanks for contacting us. Can you provide a concreate example in a real application (that is used in production) where this is happening and what would be the expected benefit for solving it? Also, feel free to send us a PR if you have enough data, you think will convince us to take it and we can continue the conversation on that PR.

ghost commented 1 year ago

Hi @petr-horny-bsshop. 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.

ghost commented 1 year ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. If it is closed, feel free to comment when you are able to provide the additional information and we will re-investigate.

See our Issue Management Policies for more information.

petr-horny-bsshop commented 1 year ago

@mkArtakMSFT We are writing a DataLayout component. It looks like this:

2023-01-09 07_28_47-Editor ikon

This component is quite complex and consists of many parts.

Changing the value in any field renders the whole DataLayout component. This is correct because another field may depend on the same property.

It would be necessary to render only those fields for which the value in the model they represent changes.

However, this would require overriding the ShouldRender method. See the code below. The component (part of it) has very many properties. Several of them are complex types. A proper implementation of ShouldRender would be non-trivial, hard to maintain, and error-prone.

Here's an advocacy for not going the ShouldRender override route and rendering the component even though it generates an empty diff:

  1. Implementing ShouldRender is non-trivial, hard to maintain and error-prone.
  2. We prefer to sacrifice some CPU power for simpler, clearer and more manageable code. After all, that's why we write code in C# and not in C++, right?
  3. The time it takes to render a component is predictable because the component is rendered on our server. The time it takes to transfer diff data from the server to the client is highly dependent on network latency and is out of our control.
@inherits BsRazorComponent
@typeparam TData
@typeparam TValue

@if(!Visible)
{
    return;
}

<div style="@GetStyle()">
    @ItemTemplate(_valueHolder)
</div>

@code {
    private readonly ValueHolder<TValue> _valueHolder = new ValueHolder<TValue>();

    [CascadingParameter(Name = nameof(BsDataLayoutBase))]
    private BsDataLayoutBase DataLayout { get; set; } = default!;

    [CascadingParameter(Name = nameof(BsDataLayout<object>.ItemTemplate))]
    private RenderFragment<IValueHolder> ItemTemplate { get; set; } = default!;

    [CascadingParameter(Name = nameof(IBsDataLayoutGroup))]
    private IBsDataLayoutGroup LayoutGroup { get; set; } = default!;

    [CascadingParameter(Name = nameof(BsDataLayout<object>.Data))]
    private TData Data { get; set; } = default!;

    /// <summary>
    /// Editovaná hodnota.
    /// </summary>
    [Parameter]
    public TValue Value
    {
        get => _valueHolder.DataSourceValue;
        set => _valueHolder.DataSourceValue = value;
    }

    /// <summary>
    /// Událost při změně editované hodnoty.
    /// </summary>
    [Parameter]
    public EventCallback<TValue> ValueChanged { get; set; }

    /// <summary>
    /// Hodnoty, které je možné využít v šabloně editoru a zobrazit např. dropdown.
    /// </summary>
    [Parameter]
    public TValue[]? Options { get; set; }

    /// <summary>
    /// Událost pomocí které je možné editovanou hodnotu validovat.
    /// </summary>
    [Parameter]
    public EventCallback<ValidationEventArgs<TData, TValue>> Validate { get; set; }

    /// <summary>
    /// Popisek prvku.
    /// </summary>
    [Parameter]
    public string? Label { get; set; }

    /// <summary>
    /// Libovolný pomocný objekt, který je možné využít v šabloně editoru.
    /// </summary>
    [Parameter]
    public object? Tag { get; set; }

    /// <summary>
    /// Šířka popisku.
    /// </summary>
    [Parameter]
    public string? LabelWidth { get; set; }

    /// <summary>
    /// Šířka editoru.
    /// </summary>
    [Parameter]
    public string? EditorWidth { get; set; }

    /// <summary>
    /// Explicitní pozice sloupce nebo rozsah.
    /// Zadávejte hodnotu jako pro css grid-column.
    /// </summary>
    [Parameter]
    public string? GridColumn { get; set; }

    /// <summary>
    /// Explicitní pozice řádku nebo rozsah.
    /// Zadávejte hodnotu jako pro css grid-column.
    /// </summary>
    [Parameter]
    public string? GridRow { get; set; }

    /// <summary>
    /// Css inline styl.
    /// </summary>
    [Parameter]
    public string? Style { get; set; }

    /// <summary>
    /// Zda zobrazovat popisek.
    /// </summary>
    [Parameter]
    public bool ShowLabel { get; set; } = true;

    /// <summary>
    /// Zda je tento prvek viditelný.
    /// </summary>
    [Parameter]
    public bool Visible { get; set; } = true;

    /// <summary>
    /// Počet řádků v případě textového editoru.
    /// </summary>
    [Parameter]
    public int Lines { get; set; } = 1;

    /// <summary>
    /// Zda je celý prvek zakázaný.
    /// </summary>
    [Parameter]
    public bool Disabled { get; set; }

    /// <summary>
    /// Umístění popisku.
    /// </summary>
    [Parameter]
    public BsDataLayoutLabelPosition? LabelPosition { get; set; }

    /// <summary>
    /// Horizontální zarovnání popisku.
    /// </summary>
    [Parameter]
    public BsDataLayoutLabelAlignment? LabelAlignment { get; set; }

    /// <summary>
    /// Regulární výraz pro validaci.
    /// </summary>
    [Parameter]
    public string? ValidationRegEx { get; set; }

    /// <summary>
    /// Zpráva při validační chybě.
    /// </summary>
    [Parameter]
    public string? ValidationMessage { get; set; }

    protected override void OnParametersSet()
    {
        base.OnParametersSet();

        if (DataLayout == null) throw new Exception("Tato komponenta musí být uvnitř komponenty BsDataLayout.");
        if (LayoutGroup == null) throw new Exception("Tato komponenta musí být uvnitř komponenty LayoutGroup.");
        if (ItemTemplate == null) throw new Exception("Šablona položek nesmí být null.");
        if (Data == null) throw new Exception($"Editovaná položka typu {typeof(TData)} nesmí být null. Label={Label}, parent={LayoutGroup}");

        _valueHolder.DataSourceValue = Value;
        _valueHolder.Label = Label;
        _valueHolder.EditorValueChanged = EditorValueChanged;
        _valueHolder.OnStateHasChanged = StateHasChanged;
        _valueHolder.Options = Options;
        _valueHolder.LabelWidth = LabelWidth ?? LayoutGroup.LabelWidth ?? DataLayout.ItemLabelWidth;
        _valueHolder.EditorWidth = EditorWidth;
        _valueHolder.ShowLabel = ShowLabel;
        _valueHolder.Tag = Tag;
        _valueHolder.ReadOnly = !ValueChanged.HasDelegate;
        _valueHolder.Disabled = Disabled;
        _valueHolder.Lines = Lines;
        _valueHolder.LabelPosition = LabelPosition ?? LayoutGroup.LabelPosition ?? DataLayout.ItemLabelPosition;
        _valueHolder.LabelAlignment = LabelAlignment ?? LayoutGroup.LabelAlignment ?? DataLayout.ItemLabelAlignment ?? (_valueHolder.LabelPosition == BsDataLayoutLabelPosition.Left ? BsDataLayoutLabelAlignment.Right : BsDataLayoutLabelAlignment.Left);

        var itemEditorTemplate = DataLayout.GetItemEditorTemplate(_valueHolder);

        _valueHolder.EditorTemplate = @<text>@itemEditorTemplate(_valueHolder)</text>;
        _valueHolder.PreviewTemplate = @<text>@_valueHolder.DataSourceValue</text>;
    }

    protected override bool ShouldRender()
    {
        // TODO po nastavení parametrů vygenerovat hash na základě všech parametrů včetně parametrů přebíraných z LayoutGroup a DataLayout a pokud se hash nezmění, tak nerenderovat
        return base.ShouldRender();
    }

    private async Task EditorValueChanged()
    {
        var args = new ValidationEventArgs<TData, TValue>(Data);

        args.Value = _valueHolder.EditorValue;

        // vlastní validace
        await Validate.InvokeAsync(args);

        // výchozí validace
        if (string.IsNullOrEmpty(args.Error) && ValidationRegEx.IsNotEmpty())
        {
            var strValue = args.Value?.ToString() ?? "";
            if (!Regex.IsMatch(strValue, ValidationRegEx)) args.Error = ValidationMessage ?? "Neplatná hodnota";
        }

        _valueHolder.Error = args.Error;

        var originalVale = _valueHolder.DataSourceValue;

        _valueHolder.EditorValue = args.Value;

        if (string.IsNullOrEmpty(_valueHolder.Error)) _valueHolder.DataSourceValue = args.Value;

        if (!Equals(originalVale, _valueHolder.DataSourceValue))
        {
            await ValueChanged.InvokeAsync(_valueHolder.DataSourceValue);
            await DataLayout.NotifyValueChanged();
        }
    }

    private string GetStyle()
    {
        var sb = new StringBuilder();
        sb.AppendIf($"grid-column:{GridColumn};", GridColumn.IsNotEmpty());
        sb.AppendIf($"grid-row:{GridRow};", GridRow.IsNotEmpty());
        sb.Append("width:100%;");
        sb.Append(Style);
        return sb.ToString();
    }
}
SteveSandersonMS commented 1 year ago

Thanks for the info.

I've investigated and it does seem like we could omit the empty diffs from the batch. We could omit it at the serialization stage (as you suggested) or perhaps even change ComponentState.RenderIntoBatch not to add the diff to the batch in the first place if it's empty. This would require some very careful consideration to make sure it doesn't violate any other assumptions around how the state is managed, cleanup is performed, etc. The reason we haven't done this in the past is that we didn't consider empty diffs to take up much space, and it seemed like a rather artificial case to be producing thousands of them. But perhaps your UI is different.

As a caution, though - it wouldn't help if your component contained any event handlers, since on each render, they produce different event handler delegates and therefore need updated event handler IDs. In other words, components with event handlers won't produce empty diffs in most cases, even if you don't see any visible change to the rendered UI.

I'll mark this as a backlog item since we have other things to work on more urgently. If this continues to receive community feedback its prioritisation could increase.

One other thing that might help in your case is https://github.com/dotnet/aspnetcore/issues/35897. Once this is implemented in .NET 8, the compression should reduce the bandwidth impact of these repeated empty blocks dramatically. This might have enough of an impact that the empty diffs become irrelevant in practice (though I still agree with the idea of omitting them in principle).

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.