-
Notifications
You must be signed in to change notification settings - Fork 10k
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
[Blazor] Improve BuildRenderTree performance by allowing the app developer to state unequivocally that specific state has not changed #26016
Comments
I've updated the ticket to refer explicitly to record types instead of just IEquatable, as the IEquatable is obviously about identity equality rather than state equality. |
Unfortunately, record types are not deeply immutable. They can contain properties that are themselves mutable. The runtime would have to walk all chains of reachable properties to see if the transitive closure is immutable (including reflecting over all private fields in the graph), which is not great. It would lead to traps like ‘adding a new private field of type ‘object’ to a seemingly unrelated type suddenly causes widespread changes to rendering characteristics’. We try to avoid designing in traps like that. |
What we have considered is an attribute like [TreatAsDeeplyImmutable] you could put on any type if you wanted the system to behave as if it was deeply immutable (whether or not it actually is). |
That would be good if you could get it into the DataAnnotations library. I wouldn't want a Blazor dependancy in my state project. What about a global hook in Blazor we can set that the ChangeDetection can ask if two values are deep equal? |
Unfortunately it’s meaningless to talk about ‘two values’ being equal because typically the old and new values point to the same object instance. The question to be answered is ‘has it mutated?’. If you really want to control the ‘should this component render now’ logic you can do so by overriding ShouldRender. |
I'd have to do that in every component, and would be unable to in third party components. If I could do it globally then I could check object instances on my records and it'd work for every Blazor component regardless of who wrote them. I think it'd be great to give this control to the app developer. Only they truly know if their state is definitely unchanged. Using this feature and proper use of immutable records would reduce my rendering by about 99%. That's a brilliant gain! I don't see any down sides. |
@SteveSandersonMS I have update the title + description to reflect a better approach resulting from our conversation here. I have also included the necessary code + an example application source. |
@mrpmorris In addition to records not being deeply immutable, you've made the (unfortunately very common) mistaken assumption that records are always immutable in the first place. So despite your effort of using records, the render logic still won't be able to get around having to assume that instances may have changed, because at the end of the day records are really ordinary classes just with more auto-generated members. |
@Joe4evr Indeed. Ultimately only the app developer can be expected to know if their types will remain unchanged, not component vendors. That's why I updated the request to allow a fixed point the dev can use to tell Blazor the state is unchanged. |
I'm not sure I understand the pushback on this. If the framework already incorporates one performance turnstile, what is the objection to adding another, especially for circumstances where clearly only the developer can say for sure? |
@kiyote There's no pushback 😄 The framework already has In this issue we are tentatively considering a built-in mechanism for denoting a certain custom type as immutable (e.g., a When it comes to your own components, you can already implement any of the above mechanisms yourself by overriding |
Hi @SteveSandersonMS I don't see pushback :) Whatever it is you choose to do, please don't make it so I have to reference Blazor from the library that holds my state as it will be UI agnostic. Or, at least don't make it the only way. A global hook that also looks for attributes would obviously do just as nicely :) |
Honestly, I'd prefer an approach that let me inject my own code to do the evaluation, rather than an annotation. I already have an analyzer that checks that things I've tagged [Immutable] are actually immutable, so I could just leverage that with "does it have the tag, or is a known immutable type?" in some routine, rather than having re-code things to utilize a new attribute. Upon reflection, registering the types in an "immutable map" would also be quite reasonable. And I suppose it would feel more like DI as well. |
@SteveSandersonMS I just spotted this attribute. Could you use this, or is it in an assembly that isn't referenced? |
@mrpmorris Maybe we could. Blazor applications already reference
Giving it meaning at runtime would not be consistent with that, so we'd at least have to bring it as a proposal to the base class library owners and ask whether they are happy to generalise its meaning, change the guidance, etc. |
Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue. This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue! |
No, don't close it you naughty robot! |
Since this issue has been locked for nearly 4 years, we’d no longer assume it reflects the current needs of Blazor developers. Plus, since it is already possible for component authors to control the re-rendering decision with arbitrary logic by overriding ShouldRender, many developers are likely to have patterns that meet these needs, or perhaps it may have turned out that this isn’t a top priority in practice. We’re definitely open to enhancing the framework in this area in the future. |
Problem
The current
ChangeDetection.MayHaveChanged
method indicates a BuildRenderTree is required if1: The type of the old / new is different
2: Or if it is not a known immutable type
3: Or if the value has changed
aspnetcore/src/Components/Components/src/ChangeDetection.cs
Line 22 in 6be5f20
The problem with this is that all objects will be considered possibly changed if they are not a known immutable type.
I now use
record
types extensively throughout my state, and the current logic will assume they mave have changed - this will result in unnecessary calls to BuildRenderTree.Request
Make ChangeDetection public and add a global hook that lets me
A: Not specify that I know if a value has changed or not
B: Indicate I know that a deep state comparison of an object is unchanged
C: Indicate I know an object has definitely changed (overrides any handler that says it is unchanged)
This will not only make it unnecessary to override ShouldRender in our app components that render objects we know haven't changed, but will also ensure that 3rd part components are aware that they don't need to re-render too.
Suggested changes
Here are some suggested changes to the
ChangeDetection
classExample use
The text was updated successfully, but these errors were encountered: