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.44k stars 10.02k forks source link

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

Closed Tragetaschen closed 3 years ago

Tragetaschen commented 4 years ago

Current status: https://github.com/dotnet/aspnetcore/issues/18919#issuecomment-823448077

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.

mrpmorris commented 4 years ago

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 commented 4 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.

stefanloerwald commented 4 years ago

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

@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 commented 4 years ago

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

stefanloerwald commented 4 years ago

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

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

simonziegler commented 4 years ago

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

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

@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 commented 4 years ago

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.

rodhemphill commented 4 years ago

Wow ... this is a shock. I'm from a xamarin background. I was listening to someone from an Angular background saying how poorly blazor performs ... and I was starting to believe him because comparing a xamarin app on an iphone to a blazor app on a hefty azure server .. the mobile app wins hands down!

I have a list of 120 cards on my page each with information about a staff member. Pushing one button to update one card causes the getters on every field on every card to be called. It takes 2 second. That's just poor architecture.

SteveSandersonMS commented 4 years ago

You get to control the granularity of change-detection and rerendering by choosing how to split up your UI into components:

So if you want the rendering not to recurse into a particular subtree, you can either ensure you're only passing primitive parameter types (e.g., int, string, DateTime) so the framework can detect if there are definitely no mutations, or in more complex scenarios you can override ShouldRender and plug in your own logic. This is almost identical to the update flow used very successfully by React.

mrpmorris commented 4 years ago

@SteveSandersonMS Consider a scenario where a component has the following

1: A parameter passed by its parent 2: An @onclick event 3: An @onmousemove event (which we don't want to cause a render)

The smallest amount of code I can think of is

bool PreventRender = false;

private void MyOnMouseMove()
{
  PreventRender = true;
}

protected override bool ShouldRender()
{
  if (!PreventRender)
    return true;
  PreventRender = false;
  return false;
}

Or instead of being imperative, we could be declarative and write one of the following

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

I prefer declarative anyway, but in this case it's also a lot less work.

stefanloerwald commented 4 years ago

I don't think the PreventRender pattern that @mrpmorris shows is even sufficient to cover all cases. When you're designing a component LibraryComponent where the users of that component provide other child elements with bindings to that component, there's no way to get this working. For example

<LibraryComponent>
   @foreach (var item in elements)
   {
      <UserComponentHere @key="item" @bind-Value="@item.X" />
   }
</LibraryComponent>

Suppose the LibraryComponent needs to re-render when elements change, but shouldn't (because of high cost) when any of the bindings fire, there's no way within the LibraryComponent to make that happen. If I could tell my users to use @bin-Value:norender="@item.X", then problem solved.

pha3z commented 3 years ago

There's a lot of discussion here about preventing rerenders on individual events. And that has required a lot of considerations. But going back to the original poster's issue, he wants to just eliminate the call to StateHasChanged() across the board for all calls to HandleEventAsync(). That seems easily configurable with a component-level setting.

Is there a global options configuration class that can be applied to all components automatically? And if yes, can a simple boolean be added to it for disabling rerenders on events for all components and allow individual components to be configured independently as well?

The per-event control is something that could be added after a more basic feature for component-level control, which requires a lot fewer considerations.

Presently, I've seen discussion on SO of at least one person suggesting to override ComponentBase to disable rerendering. I find that a bit silly for a simple on/off switch.

jhouxzirrus commented 3 years ago

Clarification question:

Even though events cause a StateHasChanged call, this doesn't actually result in any communication with the browser unless DOM diffing finds that the DOM truly has changed, correct?

stenfanloerwald said:

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

But how are those two statements both true? If the render diff is determined to be empty, then there shouldn't be anything sent to the browser and no actual re-render should occur, right?

But another question still: What happens if you fire an event on a Component Tree and then the diffing determines that just one small item somewhere way deep in the tree has changed? Is a rerender command sent to the browser with replacements for the entire tree or only the different element??

stefanloerwald commented 3 years ago

stenfanloerwald said:

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

But how are those two statements both true? If the render diff is determined to be empty, then there shouldn't be anything sent to the browser and no actual re-render should occur, right?

There are two things that have a computation cost:

  1. calculating the render diff
  2. applying the render diff

Obviously, the cost of applying an empty render diff is zero. The cost I have mentioned is for the calculation of the render diff. For that, you incur all the cost of calculating the DOM for any potentially-changed component and a subsequent comparison with the current state of the DOM. The result of that comparison will be that there is no difference. It just takes unnecessarily long to come to that conclusion.

pha3z commented 3 years ago

So in Blazor Server, it means tons of wasted computing power. In Blazor Wasm, it means wasted cpu power.

Even though a zero diff shouldn't have any impact on the on the user experience, there are plenty of applications where you would want to do a lot of computations in Blazor in response to an event and then manually send a rerender with several changes all at once. In these cases, you'd want to make sure Blazor components didn't prematurely trigger their own rerenders. In such a case where a premature diff recomputation produces a non-zero result, you're going to waste Blazor cycles AND waste browser rendering cycles.

I believe under the current theory of operation, if you respond to an event by using synchronous blocking methods, you should be able to trust that the diffing won't occur until after your event handler has returned. Is this a correct assumption???

SteveSandersonMS commented 3 years ago

What happens if you fire an event on a Component Tree and then the diffing determines that just one small item somewhere way deep in the tree has changed? Is a rerender command sent to the browser with replacements for the entire tree or only the different element??

The diff only describes the elements/attributes that were added/changed/removed. It doesn't include the rest of the component's output.

The cost I have mentioned is for the calculation of the render diff. For that, you incur all the cost of calculating the DOM for any potentially-changed component and a subsequent comparison with the current state of the DOM. The result of that comparison will be that there is no difference. It just takes unnecessarily long to come to that conclusion.

Yes, you're absolutely right about this. Of course, Blazor can't know that the output will be unchanged except by computing and diffing that output. If you, the developer, know it's going to be unchanged based on your knowledge of your rendering logic, you can use ShouldRender to suppress the update, or even suppress it by default on events as described below.

I believe under the current theory of operation, if you respond to an event by using synchronous blocking methods, you should be able to trust that the diffing won't occur until after your event handler has returned. Is this a correct assumption???

Yes. There's only one thread within any given dispatcher (that's one per circuit for Blazor Server, and one total for Blazor WebAssembly). So if your own code is running synchronously on that thread, the framework has no chance to do anything until you yield the thread.

In these cases, you'd want to make sure Blazor components didn't prematurely trigger their own rerenders

You can certainly do that using ShouldRender or as mentioned below.

Presently, I've seen discussion on SO of at least one person suggesting to override ComponentBase to disable rerendering. I find that a bit silly for a simple on/off switch.

Other options are possible too. You can disable rerendering on event handling by adding @implements IHandleEvent at the top of your component, and also adding this:

// Replace the default event-handling behavior with logic that purely invokes the event handler
// and doesn't trigger any re-rendering
Task IHandleEvent.HandleEventAsync(EventCallbackWorkItem callback, object arg)
    => callback.InvokeAsync(arg);

Whether you wish to put that on a common base class, so that all your components get this behavior, is totally up to you.

mrpmorris commented 3 years ago

@SteveSandersonMS It's interesting, and I hadn't thought of that, but I don't think there is a way to say "Render by default, but not if the event was a mouse-move on a specific component" is there?

For example, on one component I might want mousemove to repaint, but on another I might only want the x,y

pha3z commented 3 years ago

For example, on one component I might want mousemove to repaint, but on another I might only want the x,y

Based on everything Steve has said, I think a practical way to achieve this would be adding a HashSet EventsWhichSkipRerender member to ComponentBase (or your customized ComponentBase) that stores events you want to skip rendering on. In the constructor or OnInitialized() for your derived component, you add events that you don't want triggering renders.

The same pattern can be used with a ComponentBase.SkipRerenderOnAllEvents boolean member. You just set the value in your derived component constructor or OnInitialized().

The EventsWhichSkipRerender and SkipRerenderOnAllEvents members would need to be checked inside the IHandleEvent.HandleEventAsync() method as part of its normal logic. But this can be done on your base Component class. You wouldn't need to reimplement per component.

It'd be nice if something like this were made a stock feature of Blazor... presuming this isn't going to add too much complication. I don't mind frameworks that require some custom tweaking like this if it keeps the initial framework light and easy to maintain. Depends on how valuable the feature is to how many people.

pha3z commented 3 years ago

Depends on how valuable the feature is to how many people.

I want to point out that—while it might not seem like a ton of people are requesting this—there is a lot of opinion about "Blazor server being impractical" due to its spurious updates and horrible performance. I think a significant portion of the community just thinks "Its slow. It's a bad idea. Not good tech." and they stop there. Whereas, the reality is that Blazor Server could perform much better if you equip the users with better tooling and day-one information on how it can be made to run fast.

pha3z commented 3 years ago

I also want to reiterate that the numbers quoted by @stefanloerwald are mind-boggling horrible. The cost to do a DOM diff and comparison is insanely expensive. One single user on a Blazor server is one thing, but even a hundred is going to cost way excessive amounts of unnecessary computing power with all this constant diffing going on.

And for what its worth, I suspect a lot of this cost has to do with the fact that its a memory object web-walk. It would be nice if it were possible to load the virtual DOM and the generated component markups into contiguous memory blocks (presuming this hasn't been done already). The feasibility of this might be nearly impossible due to architecture convolution... but the performance improvement of contiguous memory comparisons would easily be on the order of 1 to 2 magnitudes faster.

SteveSandersonMS commented 3 years ago

I don't think there is a way to say "Render by default, but not if the event was a mouse-move on a specific component" is there?

Since you can replace as much of ComponentBase as you want, you can implement any kind of lifecycle or event handling/rerendering strategy that you want, perhaps along the lines as described by @pha3z.

However, I'll also suggest another technique. I know it's not obvious, but another (maybe more convenient) way you can control the re-rendering is to supply your own IHandleEvent instance on a per-handler basis. For example:

  1. Add this EventUtil class to your project, anywhere
  2. Now, whenever you have an event handler, you can optionally wrap it in AsNonRenderingEventHandler, without having to change anything about your component base classes

For example, in the default Counter page:

<button class="btn btn-primary" @onclick="EventUtil.AsNonRenderingEventHandler(Increment)">Increment counter</button>

@code {
    void Increment()
    {
        currentCount++;

        // Now we have to call this manually, otherwise it won't re-render
        StateHasChanged();
    }
}

... or if you want to pass event args or be async:

<button class="btn btn-primary" @onclick="EventUtil.AsNonRenderingEventHandler<MouseEventArgs>(Increment)">Increment counter</button>

@code {
    async Task Increment(MouseEventArgs eventArgs)
    {
        await Task.Delay(1000);
        currentCount += (int)eventArgs.OffsetX;

        // Now we have to call this manually, otherwise it won't re-render
        StateHasChanged();
    }
}

If you think it's uncommon that you want to suppress rendering, this is probably a very convenient way to do it.

If you want to do it all the time, then the base class approach is probably more convenient.

SteveSandersonMS commented 3 years ago

And for what its worth, I suspect a lot of this cost has to do with the fact that its a memory object web-walk. It would be nice if it were possible to load the virtual DOM and the generated component markups into contiguous memory blocks

That's exactly what we already do, mainly to minimise allocations. Check the sources for details :)

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.

pha3z commented 3 years ago

That's exactly what we already do, mainly to minimise allocations. Check the sources for details :)

And it still takes that much time to compute? Good grief!

stefanloerwald commented 3 years ago

That's exactly what we already do, mainly to minimise allocations. Check the sources for details :)

And it still takes that much time to compute? Good grief!

Let's please try to remain constructive. On Blazor server-side, the overhead is not as pronounced, obviously, as it runs at native speed. The numbers I've shared are for Blazor wasm and also depend on device.

Thanks @SteveSandersonMS for sharing the EventUtil code. It's a bit clunky, but fine as a workaround until a nicer way comes along (i.e. something like the onevent:norender I've been dreaming about).

pha3z commented 3 years ago

Let's please try to remain constructive.

Apologies! I was just quite surprised that the numbers would be that high when steps have been taken to put the data in contiguous blocks. But since you point out that the tests are specific to wasm, that certainly puts a different picture on it. I've read a lot of reports about wasm performance being sometimes hard to predict.

Also, I should have realized those were wasm tests. Now I see why you were pointing out the 60fps issue. You're saying that even if the wasm were running in a background thread (does it? I don't know enough about wasm), if it gets too slow, it will struggle to issue redraws at a pace reasonable to the browser if there's a lot of events happening in short succession.

stefanloerwald commented 3 years ago

@SteveSandersonMS I've had a quick round of experimentation with your EventUtil helper. It works as expected. However, I think a true norender feature should be implemented by not passing a reference to this to the EventCallback factory. See https://github.com/dotnet/aspnetcore/issues/24655: if the compiler would not write this into the generated code, static RenderFragments with events are possible. Currently, only non-razor-markup static RenderFragments OR razor-markup non-static RenderFragments can be done, which is a shame.

There are use cases, e.g. a

@code {
private static readonly RenderFragment<Action> default_button = (on_click_action) => @<text>
<button @onclick="on_click_action">
Default button. This is currently impossible, because the compiler will write "this", but the RF is static.
We don't know yet where this fragment will be used, so we don't even know which component should be implicitly re-rendered. 
This field certainly doesn't have to be created every time for each component instance.
</button>
</text>;
[Parameter] public RenderFragment<Action> ButtonTemplate {get;set;} = default_button;
}
SteveSandersonMS commented 3 years ago

@stefanloerwald I agree. I was only offering EventUtil as a short-term suggestion. It would be better to have proper compiler support to reduce allocations, as you've described. This issue is in the "Next spring planning" milestone currently so it will later become a candidate for the 6.0 planning.

MithrilMan commented 3 years ago

Question about WASM performance in this scenario: is the diff process happening into native wasm or does it involve the blazor IL interpreter? Question can be restricted to: once AOT will be a feature (not IF, because that would be the only way for blazor wasm to be usable in serious project) will performance be closer/better than current angular/vue/whatever framework around?

About DOM render, if I understood correctly, currently WASM doesn't have native access to it so there is always some perf degradation because of needed interop, correct?

Thanks

SteveSandersonMS commented 3 years ago

@MithrilMan The diffing happens in .NET code. So today it's interpreted, and in .NET 6 you can AOT it.

currently WASM doesn't have native access to it so there is always some perf degradation because of needed interop, correct?

There's no reason why calls from WebAssembly to JavaScript fundamentally have to be a lot slower than calls from JavaScript to JavaScript. V8 already makes this really fast. And since Blazor is based on sending whole batches of UI updates at once (the output from the diff), it's not a chatty API - we're likely only making a single call into JS per user interaction. You wouldn't be able to measure that overhead.

mkArtakMSFT commented 3 years ago

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

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

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

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

https://github.com/dotnet/aspnetcore/issues/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 commented 3 years ago

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

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

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

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

PR here: https://github.com/dotnet/aspnetcore/pull/31756 Issue here: https://github.com/dotnet/aspnetcore/issues/31755

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

Kylar182 commented 3 years ago

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

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.

captainsafia commented 3 years ago

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 (https://github.com/dotnet/AspNetCore.Docs/pull/22091).