Modify `ChangeDetection.IsKnownImmutableType` to support custom immutable types w/o perf impact

[alexyakunin]

Summary

The last line in https://github.com/dotnet/aspnetcore/blob/main/src/Components/Components/src/ChangeDetection.cs#L45 checks for implementation of a very specific IEventCallback interface. If it's implemented, the type is treated as immutable.

Let's make IEventCallback a descendant of tagging IBlazorImmutable (the name is questionable, maybe we need IImmutable in .NET?) and use it instead in IsKnownImmutableType code.

Motivation and goals

There are cases where you'd rather detect parameter change with nearly zero false positives vs dealing with more false positives, which typically trigger re-renders or other logic that depends on component parameters. So it's good to have at least one easy option allowing you to declare a parameter type that relies on custom equality.

In scope / out of scope

Not sure what to write here: assuming we're talking about an interface specific to Blazor only (~ IBlazorImmutable), the proposed change is quite isolated.

Risks / unknowns

I don't see any risks except the scenario when developers implement IBlazorImmutable w/o weighting the cost of equality check for this type.

Examples

public record UserId : IBlazorImmutable { ... }
public record Avatar : IBlazorImmutable { ... }
public SmallSet<T> : IBlazorImmutable { ... }

Detailed design

// New interface
public interface IBlazorImmutable;

// Change:
public interface IEventCallback: IBlazorImmutable { ... }

// Change on L45 in ChangeDetection.cs:
// from:
|| typeof(IEventCallback).IsAssignableFrom(type);
// to:
|| typeof(IBlazorImmutable).IsAssignableFrom(type);

[javiercn]

@alexyakunin thanks for contacting us.

This algorithm is conservative and not extensible on purpose as allowing extensibility will result in situations where incorrect implementations cause the algorithm to not detect changes and not trigger additional renders when there are in fact changes, which will result in really hard to debug issues.

You can already control this behavior in your own component by overriding SetParametersAsync and avoiding a render if you deem the parameters haven't changed, but the default algorithm is conservative and flavors correctness over "efficiency". There's also another trade-off which is that the more things we check, the slower it becomes for every component to render, even if it uses existing parameter types.

We might in the future extend support for some types where the compiler provides the equality implementations (like records) but that's not currently a priority.

[alexyakunin]

@javiercn sorry, but I can't agree with you:

1. Blaming yourself in allowing others to make mistakes is kinda bad excuse for making something closed rather than open.
2. What's available now is, honestly, a bad option, coz it makes people build way more hacky solutions for problems they still have to tackle.

I certainly know about SetParametersAsync - https://github.com/ActualLab/Fusion/blob/master/src/ActualLab.Fusion.Blazor/Components/FusionComponentBase.cs#L23 ; feel free to check out how deep is the rabbit hole we dug w/ customizable parameter comparison. And all of this is necessary mainly to make sure the state of certain components is recomputed only when it makes sense - without too many false positives. And it's easy to notice there is way more code than it should be - thanks to, as you've said, "the algorithm that is not extensible on purpose".

Not sure if it's clear, but the fact you make it closed like this doesn't solve any problem - it just creates a bigger one. Overriding SetParametersAsync like we already do is one of such ways. But notice that nothing prevents us from "implementing" fake IEventCallback as well to make certain parameter types compared via regular equality. And as soon as this "solution" becomes a well-known one, the whole idea of closing the API like this becomes way more arguable than it seems now.

CC @SteveSandersonMS ?

[javiercn]

@alexyakunin thanks for the additional details.

A few things:

• I don't think you should "abuse" IEventCallback in that way. We aren't going to block you but be 100% sure that it's not a supported scenario and that it might break in the future.
• If we wanted to support custom equality, we would probably call Equals rather than our own interface.
• I've looked at the implementation you provided. It's unclear to me why you are comparing the properties instead of capturing the values from the ParameterView into a dictionary and comparing them against the new ones.

We might disagree on goals here:

• Triggering an extra render might introduce a performance penalty when the values have indeed not changed.
• Not triggering a render will translate in an incorrect behavior of the app, that is also, really hard to debug.

In this case, we favor correctness over potential performance gains because we believe is much worse to run into a situation where you have a hard to debug issue than the cost of potentially triggering a few additional renders.

If you have components where rendering is expensive you can control when they render via ShouldRender or overriding SetParametersAsync. That's the way we recommend people handle this situation, as opposed to interfering with the diffing algorithm that has a much broader impact.

[alexyakunin]

@javiercn lol, I didn't notice IEventCallback is internal - i.e. yeah, it's hard to abuse it that way.

If we wanted to support custom equality, we would probably call Equals rather than our own interface.

IMO it's totally reasonable to make sure the default equality isn't used for parameter comparison:

• Yes, most of data structures aren't immutable, so it just doesn't make any sense to use it
• And yes, Equals can be expensive.

So IMO what's already done is absolutely correct. And my suggestion doesn't break any benefits of existing soution.

• The tagging with IBlazorImmutable is a very explicit action.
• None of existing types is tagged with this interface.
• If you tag a type with IBlazorImmutable, it's reasonable to assume you know the consequences of this.
• And there is no computational cost penalty.

In other words, I really see no cons.

[alexyakunin]

• I've looked at the implementation you provided. It's unclear to me why you are comparing the properties instead of capturing the values from the ParameterView into a dictionary and comparing them against the new ones.

All depends on the number of such parameters you actually have: if it's about just a few components & params, of course you can manually copy the old value for each param & compare it with the new one in SetParametersAsync to decide whether the change had happened.

But what if it's about nearly every parameter in ComputedStateComponent<T> descendant? There can be hundreds such descendants, each with a few params they use in ComputeState(...). So when these params change, the state has to be recomputed, which typically triggers re-render.

You can certainly try to manually copy & compare all of such params, but this is definitely more error prone than using a solution which addresses the root cause & requires no manual handling of each specific case.

[alexyakunin]

We might disagree on goals here:

• Triggering an extra render might introduce a performance penalty when the values have indeed not changed.
• Not triggering a render will translate in an incorrect behavior of the app, that is also, really hard to debug.

Yes, I fully understand this. Just want to point out that suggested change doesn't break these assumptions.

If you have components where rendering is expensive you can control when they render via ShouldRender or overriding SetParametersAsync. That's the way we recommend people handle this situation, as opposed to interfering with the diffing algorithm that has a much broader impact.

That's exactly what we do. You may notice there is e.g. https://github.com/ActualLab/Fusion/blob/master/src/ActualLab.Fusion.Blazor/Components/ComputedRenderStateComponent.cs , which cancels renders unless the state it computes gets changed. It's not super straightforward from the code, but ultimately, it assumes that component's State (it's what ComputedStateComponent<T> computes/updates when State dependencies change) is the only data source for its render logic. So unless it changes, it doesn't have to re-render anything. And it uses State.Snapshot to track the changes.

Long story short, we know how it works pretty well.

[alexyakunin]

I also want to highlight that what I suggested is definitely sub-optimal:

• It allows to override the "blazor equality" only for your own types
• So if you want to override it for something else, it won't work. And this is actually important even in our own case.

And IMO this is a totally valid reason to reject this proposal as "not good enough". But let's come up with something that's good enough in this case. I hope it's clear that having an ability to override "blazor equality" is important, and personally I would be fine with any better option (e.g. making IChangeDetection DI-injectable, etc.).

I also think that having ~ public interface IChangeDetection + its default implementation is better than what we have right now (i.e. internal ChangeDetection type). I bet that maybe 80% of Blazor devs simply don't know how it works, and that's solely because it's buried deeply in Blazor internals. But if you want to eliminate excessive renders, you absolutely need to know how it works.

And that's where your "we need reliable re-renders on changes, so let's make Blazor equality 'stupid' and close it" starts to hurt anyone who crafts something bigger than a toy app:

• You know Blazor (esp. in WASM mode) isn't the fastest UI framework on the block.
• Nevertheless, you advocate a decision that prevents us from solving one of perf. issues (excessive re-renders / re-computations) at scale.

[javiercn]

@alexyakunin thanks for the additional details.

I think we disagree at which level this should happen. For us:

• The decision on when to render belongs to the component itself, not outside of it.
• The change detection we perform during diffing is an optimization to avoid setting parameters in common cases where we know they haven't changed. It's meant to be deterministic and simple, and favor correctness over anything else.

Some more details:

• If we were to provide an IComponentParametersComparer service that you could register on DI to provide your own implementation, we would open the door for anyone changing when/how third-party components render.
  • This would mean that component vendors and authors would have to account for these situations in their component's implementation.
• If we were to provide an interface that you could implement on a type, you could not change existing types, but you could still affect the rendering characteristics of generic components as the diffing would be influenced by the implementation on the given generic value.

Rendering is a very sensitive area of the codebase and a hot path, having other code running there creates all sort of opportunities to issues, from issues like "Why is my component not re-rendering" to "Why is my app suddenly slow" that can happen in very non-obvious ways.

• You use a type with a bad/slow implementation.
• You bring in your own change detection logic with bugs/performance issues.

The current approach offers predictability and stability both in terms of performance and when the component receives parameters. Any decision on when the component renders need to be performed at the component level itself, not on a central place that can have side-effects on the way other components receive parameters.

If we were to do something in this area, we would simplify the process of implementing this on a per-component level, but we wouldn't change this at the diffing level.

To follow with the example, you provided and as an alternative approach, source generation could be used to generate a method to perform the comparisons. Note that the implementation can be potentially simpler than the current one you have. You'd need to compare only parameters that aren't supported out of the box, not all the parameters, because if all supported parameters are equal, the component wouldn't receive a new set.

[alexyakunin]

Based on what I just read, my original suggestion w/ IBlazorImmutable fully fits the bill:

• Unless you're a developer authoring the component, you can't influence on how the comparison works for it. Ok, unless we're talking about components with generic parameter types, which are pretty rare.
• There is no way to change the comparison logic on the global level.

In other words, I don't understand your argument. Its applicable for another option I mentioned (extracting IChangeDetection), but not for the original one.

And... I'm not sure if you noticed, but I very explicitly saying that you advocate nearly this policy:

Developers: 
- Hey, why you gave us a hand saw while we clearly need a chainsaw?
Microsoft:
- Sorry, I can't give you a chainsaw, because you can cut your fingers. That doesn't mean I think you're dumb, of course.
Developers:
- But we need to cut through the forest! And it's our fingers, not yours - let us decide whether to risk them or not, ok?
Microsoft:
- Sorry, I can't see the forest in the toy apps and demos I've built for you.
Developers:
- Well, your 10K LOC demos aren't our 1M LOC codebases. That's why you don't see a plethora of other problems too.
Microsoft:
- I still insist my decision is correct. Also notice that I let you use a knife in addition to hand saw.
...

And speaking of source generators:

source generation could be used to generate a method to perform the comparisons

Even if we put aside all the pain and suffering you need to go through to implement a single one (believe me, I know what I'm talking about), I would be delighted to see A SINGLE source generator producing the source based on .razor / Blazor component source.

AFAIK there is a plethora of problems with this:

• AFAIK source generators can't consume the output of other source generators.
• Even the ordering is a problem: https://github.com/dotnet/roslyn/issues/57239 ... And tons of other issues you can easily Google.

[javiercn]

@alexyakunin thanks for the additional details.

Based on what I just read, my original suggestion w/ IBlazorImmutable fully fits the bill:

I don't think this is the case for two reasons:

• Libraries can still provide types that implement IBlazorImmutable that when used on components will have an impact.
• We put generic types as an example, but any component is subject to this, since the parameters are essentially a Dictionary<string,object>.

As for the source generators bit, I just wanted to point out as a potential future alternative. If we were to bake something like that in, we would integrate it directly within the Razor Compiler or solve the points that you made.

Even with the issues SG have today when dealing with Razor code, it's possible to solve that by moving the property definitions to a Code Behind file. I'm noy saying that it's ideal, just that it's possible.

As for the other comment, we don't see it this way, let me try and explain.

Developers: 
- Hey, why you gave us a hand saw while we clearly need a chainsaw?
Microsoft:
- Sorry, I can't give you a chainsaw, because you can cut your fingers. That doesn't mean I think you're dumb, of course.
Developers:
- But we need to cut through the forest! And it's our fingers, not yours - let us decide whether to risk them or not, ok?
Microsoft:
- Sorry, I can't see the forest in the toy apps and demos I've built for you.
Developers:
- Well, your 10K LOC demos aren't our 1M LOC codebases. That's why you don't see a plethora of other problems too.
Microsoft:
- I still insist my decision is correct. Also notice that I let you use a knife in addition to hand saw.
...

• We understand that you have a big solution/app, but that is simply not representative of most Blazor apps out there.
  • At some point there is a trade-off between design choices and scale, typically very large-scale apps require custom solutions.
• We don't believe excessive re-renders because of parameter comparisons represent a problem that spreads across many components in a given app.
  • It's typically focused on a few components that have an expensive render cost and that can be optimized individually.
  • Even if you are able to optimize some of your parameters, re-renders are likely to happen the moment you receive a RenderFragment, so it's not something you can broadly take advantage of.

Note also that this is not about the size of the app on LOC, but about the complexity of the UI (Number of components rendered on screen).

Finally, I would say that we are talking about performance without having any explicit numbers attached to it (For example, with a scheme like this, how many components avoid re-rendering, and how much CPU time / frames per second we gain).

We can theorize of possible fixes and how they will impact perf, but what might impact an app, might not impact many other apps out there and might be in fact harmful, and offering such a general solution like this can also be equated to killing flies with cannonballs.

In other words, for us to be open to making this core piece extensible we would need:

• Evidence that it significantly reduces the renders across a wide variety of apps.
• Evidence that such render reduction is spread across many components and not a small percentage within the app.
• Evidence that such a reduction has a positive impact in user facing metrics (FPS/CPU time).

I understand that this is not what you want to hear, and that it doesn't align with your views, but I hope that you understand that as framework authors we put in contrast other principles/values when we make design trade-offs that might not be weighted in the same way according to your own views.

[alexyakunin]

We don't believe excessive re-renders because of parameter comparisons represent a problem that spreads across many components in a given app.

I love your level of confidence :)

Ok, maybe it makes sense to point to one obvious case: typed identifiers.

• Record structs are ~ zero-cost abstraction allowing you to wrap e.g. strings to UserId. And the later are much more convenient to use - e.g. you won't pass UserId to a method expecting OrderId.
• And if you have these types, it's natural to use them as component parameters, right?

Now ask a random Blazor developer about the effect of trying to do this - at scale.

[alexyakunin]

Libraries can still provide types that implement IBlazorImmutable that when used on components will have an impact.

YES, AND THAT'S EXACTLY THE REASON TO ADD IT!

I feel like I talk with ChatGPT here. Or maybe there is an unspoken rule in Microsoft, which goes like this: "If you're brave enough to suggest some change, make sure it changes nothing. Absolutely. Otherwise, it's a no-go."

It's amazing to see that at the very same company one group of people work on language with a decently complex type system that brings type safety & lots of other benefits, if you use it right. And there is another group, who literally say, "We're absolutely sure a dozen of primitive types we pre-selected covers every single case you can dream of. Oh, wait - we almost forgot to add Guid there!" , and all of this is justified by... Presumed stupidity of an average developer, right?

Sorry, but "640KB should be enough for everyone" is way more logical than this.

[alexyakunin]

The more I think about this, the worse it becomes.

Did you guys ever dream of making Equals and GetHashCode sealed? Please be honest. Just think about the amount of pain a single override like GetHashCode() -> Random.Shared.Next() may cause.

[dotnet-policy-service[bot]]

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.