Skip to content

Add a way to avoid component re-rendering caused by events to Blazor components ("pure event handlers") #18919

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

Closed
Tragetaschen opened this issue Feb 10, 2020 · 45 comments
Assignees
Labels
affected-most This issue impacts most of the customers area-blazor Includes: Blazor, Razor Components Docs This issue tracks updating documentation Done This issue has been fixed 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 severity-minor This label is used by an internal tool
Milestone

Comments

@Tragetaschen
Copy link
Contributor

Tragetaschen commented Feb 10, 2020

Current status: #18919 (comment)

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

My Blazor component tree is connected to the business logic of the running application and represents its current state. There are a few event handlers (@onclick) and these directly call into the business logic to have an effect there. Any change of state in the business logic in turn is pushed to the Blazor components and cause them to rerender.

All the event handlers in the Blazor components are "pure" in the sense that they never change any state in the component. The current implementation of Blazor components though unconditionally re-renders (StateHasChanged) after the event callback has run even though this really isn't necessary for my components. A bit worse: The components are rendered with the old state after the event handler and then immediately the new state from the business logic arrives and causes a render for the new state.

Describe the solution you'd like

I pretty much look for a way to avoid this StateHasChanged call.

Currently I'm avoiding the rerender caused by this call by managing a shouldRender flag within my component and toggle it depending on what happens:

protected override void OnParametersSet() => shouldRender = true;

protected override bool ShouldRender()
{
    if (shouldRender)
    {
        shouldRender = false;
        return true;
    }
    else
        return false;
}

or

InvokeAsync(() =>
{
    shouldRender = true;
    StateHasChanged();
    shouldRender = false;
});

But this code is brittle, not very intuitive and hard to link to the reason why it's there, because I have to put it on the non-event side of things.

@Tragetaschen Tragetaschen changed the title Add a way to avoid component re-rendering caused by events ("pure event handlers") to Blazor components Add a way to avoid component re-rendering caused by events to Blazor components ("pure event handlers") Feb 10, 2020
@mkArtakMSFT mkArtakMSFT added area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Feb 10, 2020
@mrpmorris
Copy link

I would like to be able to specify this conditionally. Something like the following would be great.

@onmousemove:preventStateHasChanged
@onmousemove:preventStateHasChanged=true
@onmousemove:preventStateHasChanged=SomeCSharpFieldOrMethod() 

@ghost
Copy link

ghost commented Jul 23, 2020

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.

@stefanloerwald
Copy link

stefanloerwald commented Aug 10, 2020

Please consider implementing norender by removing the reference to this in the EventCallback. This enables storing RenderFragment with event callbacks in static variables: #24655

@stefanloerwald
Copy link

@SteveSandersonMS I very often run into performance issues that boil down to unnecessary render cycles introduced by the implicit render on @bind and events. Could this be worth tackling as part of the perf improvements for .NET 5? As a library author, it would be much easier to use a norender flag on event callbacks and tell users to do the same, than to try and work around the rendering with making ShouldRender return false ever so often.

@jspuij
Copy link
Contributor

jspuij commented Aug 18, 2020

@mkArtakMSFT Please add the "Help Wanted" label so someone can pick it up in a sprint :-)

@stefanloerwald
Copy link

I've created a sample repo that demonstrates the overhead quite clearly: https://github.com/stefanloerwald/blazor-bind-overhead

On SSB, the delay is just about noticable, on CSB there's no way you'll miss it.

Before someone says this example is contrived: Of course it is. Nobody will ever want 60000 buttons next to each other, even if 59999 are not rendered. The markup of the child node is deliberately simplistic. Note though that when the child component is more complex, the delay would increase too, making the delay noticable with fewer children. More complex scenarios are e.g. tables (data grids), diagrams, charts, etc.

@stefanloerwald
Copy link

A demo for CSB can be found here: https://stefanloerwald.github.io/blazor-bind-overhead/

@simonziegler
Copy link

#24599 looks like it has similar issues to this. Specifically about StateHasChanged causing first a render with old values followed by one with new values. As you can see from #24599 this causes misbehaviour with two way bound components.

@quajak
Copy link

quajak commented Aug 18, 2020

I have also been running into unwanted rerenders when using binding and event handlers, would love to be able to control the behaviour without having to rely on ShouldRender.

@stefanloerwald
Copy link

@danroth27, @SteveSandersonMS

Any chance you could please give this some attention? In terms of performance, this has quite the potential to make an impact.

I've just updated the demo page and I think it shows that the issue is quite severe. Please have a look at https://stefanloerwald.github.io/blazor-bind-overhead/ and corresponding repo. The overhead per component is around 0.28ms upwards (higher overhead for more complex markup, of course, even if the resulting render diff is entirely empty).

@stefanloerwald
Copy link

As I was asked on a different channel about the seemingly low number of 0.28ms: Yes, this is the correct unit and the decimal point is in the right place.

However, this isn't a low figure at all, considering that this is per component for the only effect of determining that the render diff is empty.

In terms of keeping applications smooth, one could consider the target of 60fps. With about 60-100 tiny components re-rendered without any visual change, 16ms are easily wasted by re-rendering nothing. So doing anything meaningful on top of that means the actual FPS drops below 60.

So in conclusion: 0.28ms isn't much on its own, but it adds up quickly to noticable delays.

@mkArtakMSFT mkArtakMSFT added the Needs: Design This issue requires design work before implementating. label Mar 19, 2021
@mkArtakMSFT
Copy link
Member

Moved the is into 6.0-Preview4 to design the solution. We will decide later when we will implement the proposed design.

@daniel-p-tech
Copy link

As much as I love Blazor, poor performance of complex components and unnecessary re-rendering is concerning.

I am designing a very lightweight grid that needs to be capable of displaying up to 500 rows per page (this is per explicit customer's request so I have to accommodate).

Best case scenario is that the component is rendered only once based on user's action (e.g. selecting a row which has to update the style of active row and therefore refresh the component). There is a noticeable lag between the time the user clicks on a row and when the row changes color.

Other scenarios are even worse. Please refer to the following code snippet:

<table class="table @ComponentService.BootstrapTableSizeModeCssClass @CssClass mb-0">
    <thead>
        <tr>
	...
        <tbody>
            @foreach(var item in ItemsView)
            {
                <tr @onclick="@((e) => OnRowClick(e, item))" @ondblclick="@((e) => OnRowDoubleClick(e, item))" class="@GetRowCssClass(item)" @key="item">
                    @foreach(var column in Columns)
                    {
                        @if (column.CellTemplate == null)
                        {
                            <td scope="row" @onclick="@((e) => OnCellClick(e, item, column))" class="@column.CellCssClass" style="@GetCellStyle(column)">@column.GetItemValueAsString(item, column)</td>
                        }
                        else
                        {
                            <td scope="row" @onclick="@((e) => OnCellClick(e, item, column))" class="@column.CellCssClass" style="@GetCellStyle(column)">@column.CellTemplate(item)</td>
                        }
                    }
                </tr>
            }
        </tbody>
        private async Task OnRowClick(MouseEventArgs mouseEventArgs, TItem item)
        {
            m_shouldRender = false;
            await RowSelected.InvokeAsync(new(mouseEventArgs, null, UIElementActivateType.Mouse, item));
	    ...
	}

        private async Task OnRowDoubleClick(MouseEventArgs mouseEventArgs, TItem item)
        {
            m_shouldRender = false;
            await RowDoubleClicked.InvokeAsync(new(mouseEventArgs, null, UIElementActivateType.Mouse, item));
        }

        private async Task OnCellClick(MouseEventArgs mouseEventArgs, TItem item, BlzGridBaseColumn<TItem> column)
        {
            m_shouldRender = false;
            await CellClicked.InvokeAsync(new GridCellEventArgs<TItem>(mouseEventArgs, null, UIElementActivateType.Mouse, item, column));

            if (column is BlzGridEditColumn<TItem>)
            {
                await InvokeCustomPopupInitialize(mouseEventArgs, null, UIElementActivateType.Mouse, item, GridCustomPopupEditActionType.Update);
            }
            else if (column is BlzGridDeleteColumn<TItem>)
            {
                await RowDeleted.InvokeAsync(new(mouseEventArgs, null, UIElementActivateType.Mouse, item));
	    }
	    ...
        }

When a user clicks on a certain table cell, up to 3 events may get triggered. Depending on what the consumer of the grid component (parent component) needs to do in these various click events, refreshing the parent component can cause additional rendering of the grid component. Even with my various attempts to minimize re-rendering (without sacrificing grid consumer's developer experience), there are instances when the grid is refreshed 4 times. At that point the user experience with over 200 rows on a high end PC becomes unacceptable. And 200 rows is really a very small number...

Ideally, I want to be able to somehow say in a simple declarative way to render the grid only once after all events are handled. If the parent component needs be updated inside any of the event handlers, the child grid component should not be refreshed. As far as I can tell this is impossible to implement.

The only realistic option seems to be to massively speed up component rendering and the diffing algorithm. Do you have any benchmarks on how much the AOT is going to help? As in 5x, 10x or hopefully something close to 100x improvement?

Are there any lessons to be learned from Svelte? I'm not very familiar with the framework but they claim to bypass the diffing completely and only update relevant DOM elements directly. Can the compiler be of any help?

Again, I greatly enjoy working with Blazor, this is the best tech on .NET stack that Microsoft has developed in years (maybe a decade), but the performance is a real problem.

Thank you for your attention.

@stefanloerwald
Copy link

@daniel-p-tech you might benefit from using Virtualize (https://docs.microsoft.com/en-us/aspnet/core/blazor/components/virtualization?view=aspnetcore-5.0). It is unlikely that 500 rows will be actually simultaneously visible (unless they have practically no height or the page is viewed on a ridiculously tall monitor), so you don't actually need to produce that many DOM elements.

@jspuij
Copy link
Contributor

jspuij commented Apr 7, 2021

@daniel-p-tech Don't use anonymous lambda's that capture variables inside a loop as event handlers in components.

e.g.

@onclick="@((e) => OnRowClick(e, item))"

item is captured by the lambda and will instantiate a new instance of an anonymous inner class inside the foreach loop, which gets its own ID and causes every row to rerender on every paint of the component. See this issue for the explanation:

#17886

Instead, add the event handler to the item, so that the code will read:

@onclick="item.OnRowClick"

That way no lambda needs to be created on an inner class and no repaints for all rows happen.

I know that the docs state that it can be done, but performance will suffer.

@SteveSandersonMS
Copy link
Member

@daniel-p-tech I agree with the comments from @stefanloerwald and @jspuij, and would also recommend you check https://docs.microsoft.com/en-us/aspnet/core/blazor/webassembly-performance-best-practices?view=aspnetcore-5.0 for further guidance on producing better-performing UI structures.

@daniel-p-tech
Copy link

@stefanloerwald we decided against virtualization early on and opted for paging instead - virtualization has some limitations and IMHO doesn't provide adequate user experience (just a personal preference) - it's something I would only consider if I had to display thousands of rows rather than 500 (per page) at most.

@jspuij thanks for the tip - I wasn't aware of this. However, your approach would require quite a bit of redesign of my component - I would have to keep a custom class that besides Action property keeps a reference to the actual item that the consumer of the grid specified in Items collection. I'm not sure how it would work with the grid consumer adding items etc. Honestly it seems as more of a hack than a clean solution that should be provided by the framework. I believe @mrpmorris shares the same sentiment.

@SteveSandersonMS I did read the linked document before posting the original comment. I just don't see how I can prevent the component to be unnecessarily rendered several times and I don't think the proposed solution using EventUtil would make much difference in my case. Is AOT going to make a significant difference?

I'd be more than happy to share a minimal repo with my grid if some of the Blazor gurus on this thread would be willing to take a look at it and critique my solution.

Thank you.

@stefanloerwald
Copy link

@stefanloerwald we decided against virtualization early on and opted for paging instead - virtualization has some limitations and IMHO doesn't provide adequate user experience (just a personal preference) - it's something I would only consider if I had to display thousands of rows rather than 500 (per page) at most.

There is nothing stopping you from using both paging and virtualization. You fetch your 500 items for one page in one go, then populate the table using virtualize. The DOM element count will be low (hence fast), and loading the data within one page is also fast, because the data is already local.

@jspuij
Copy link
Contributor

jspuij commented Apr 13, 2021

@daniel-p-tech Just a heads up that I devised a possible fix for the closure issue:

PR here: #31756
Issue here: #31755

It's not clear whether it makes the cut, but at least I gave it a try ;-)

@Kylar182
Copy link

I've been using Action On Change and Subscription with Singleton Dependency Injection to solve this issue but Pure event handlers would be preferable as it would likely require less code. Subscribing

@radderz
Copy link

radderz commented Apr 13, 2021

What about adding an args parameter to the EventCallback delegate type that presents an args object, which has a suppress render property? Similar to something like MouseEventArgs where you can set IsHandled type values to suppress further callbacks.

i.e.

EventCallback<T, EventArgs> or similar, another option would be for you to return "true" to suppress the redraw, but this wouldn't be as good for async type methods.

@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 19, 2021
@captainsafia captainsafia added Docs This issue tracks updating documentation and removed Needs: Design This issue requires design work before implementating. labels Apr 19, 2021
@captainsafia
Copy link
Member

Hello folks -- thanks for all the lively discussion on this thread and others.

We've spent some time discussing this and have decided not to pursue an API change for this at the moment. For most of the scenarios, implementing IHandleEvent within the component that contains the target event is sufficient for most scenarios. This is now documented in our performance docs: https://docs.microsoft.com/en-us/aspnet/core/blazor/webassembly-performance-best-practices?view=aspnetcore-6.0 or the PR above (dotnet/AspNetCore.Docs#22091).

@ghost ghost added Done This issue has been fixed and removed Working labels Apr 20, 2021
@danroth27 danroth27 removed the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Apr 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-most This issue impacts most of the customers area-blazor Includes: Blazor, Razor Components Docs This issue tracks updating documentation Done This issue has been fixed 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 severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests