Aspnetcore: Blazor change detection too liberal

[harleydk]

The change detection seems to me a bit liberal (for lack of a better word). Given a list of N instances of some POCO model, each handed to an individual component, changing just one of them causes the entire set of components to re-render.

I made a fiddle to demonstrate the issue: https://blazorfiddle.com/s/fla3gm2q

200 instances in the list, provided (individually) to 200 sub-components. Changing just the first one causes 200 calls to OnParametersSet - I was hoping for just one.

(Ideally I'd like to be able to update a specific sub-component, but https://github.com/aspnet/AspNetCore/issues/13358. Pardon me pressing that issue, I mean no offence).

Thank you in advance for your consideration.

[RemiBou]

Maybe this can help https://docs.microsoft.com/en-us/aspnet/core/blazor/components?view=aspnetcore-3.0#use-key-to-control-the-preservation-of-elements-and-components

[harleydk]

@RemiBou Thanks, but not quite what I was looking for.

Further to the point, the discussion in regards to my issue came up in a user-group of which I'm a member, I'm trying to promote Blazor. The question was raised, "why are CPU-cycles being wasted in touching 199 sub-components , when quite obviously no changes are made to them?!" I hadn't the answer to that, and it even seemed like a very reasonable concern.

I can appreciate it might be a complicated issue, after all it's possible to attach quite complex objects as parameters. But it shouldn't be impossible to detect those changes and, besides, my audience certainly expected that to be the case. So maybe it's more of a communication-issue. The expected behavior seemed to be something along these paraphrasing lines, "I hand the data to the component and expect it to react when the data changes, I do not expect it to react when the data does not change."

[mkArtakMSFT]

Thanks for contacting us, @harleydk.
We will look into this to see how we can improve this area to make it simpler to use in the near future.

[Andrzej-W]

@harleydk I think it is not a trivial task to optimize this at the framework level. As you have written we can use complex objects as parameters. Those objects can have references to other objects and so on. To be able to detect changes framework should make a deep copy of every parameter and then compare current values to old values. Take into account that you can inject services to your components and render some values returned from these services. In my opinion only component author can make responsible decision and you can override ShouldRender method to do this.

[harleydk]

@Andrzej-W I respectfully disagree - even with complex objects, with N levels of sub-references, it should be a - relatively - simple (in implementation terms) algorithm, and a fast comparison. ShouldRender wouldn't do much good in my current situation, it brings the change-detection responsibility to the child-component. And maybe I'm the one who implemented the parent-component, but not the child-component - so I need not necessarily be the one who can make the responsible decision, you mention, on the other component author's behalf. ShouldRender, I'm not at all a fan of that construction, by the way. It feels like something - my personal opinion only - that was bolted on to solve a problem the framework should take care of.

Like I said, maybe it's more of a communication issue. So I presented Blazor to this group I'm in, and they see the OnParametersSet being touched for apparently no reason, and it suddenly gets much harder to promote Blazor with these folks.

[Madhust]

+1 for this issue. I have faced similar issue that OnParamterSet is invoked so many times and also i find it hard to get the reason for the re-render. So i had to manually check/compare the values in the OnParameterSet.

[Liander]

@harleydk The calls to OnParametersSet is a bit confusing in this situation because if you set a break-point on the parameter you have you will see that there are no parameters set and no calls to DataPiece-set in the BidAskComponent when you click your button. Right now, any existence of a parameter (which are not-null..) of reference type is making all decisions about the render-condition being passed on to the component to decide, which is not obvious, and I agree that it is not a great developer experience. The consequence is that even if you set up your own change detection for the reference type you will miss the change detection for the parameters of primitive types the framework has done and you will have to do the same work for those also.

It can also give surprising side effects. You get different behavior depending what parameters you set. Let say you transform your component to use primitive parameter types of Bid and Ask, then you can see in this fiddle that it will only render the changed item: https://blazorfiddle.com/s/gsxywn2w
But then you would expect that if you pass a delegate OnChange to that component you would only get one changed item… you don’t (because it is a non-null reference type). You can see the side effect by uncommenting the call in the same fiddle.

Treating what has changed really depends on context, like saying my delegate will never change, etc. I think what is missing is the ability to specify (and handle) additional options of the change condition that make sense in a particular situation. I will explain in the next post as soon as I get time to simulate it in fiddle.

[Liander]

Today you can test with speed render-trees of primitives, but it gives up on all references (when set), which I think should be improved. See my previous post above.

I suggest introducing a setting on the Parameter attribute to specify choice of change detection with the options: [Parameter(ChangeDetection.Default | OnInitialization | ByReferenceValue | ChildPropertyChange | TrackChangeProperties | UsingCustomEquals)]

With the following meaning:
ChangeDetection.Default – The current behavior. For reference types (that are not of a common set of immutable types such as date, string, etc) it means always true.
ChangeDetection.OnInitialization – To trigger only on initialization. It is likely the wanted setting for delegates.
ChangeDetection.ByReferenceValue – It would be used for immutable objects or objects you want to see as immutable in the context used. (I used it differently from Equals because we know this is fast and it doesn’t potentially hide what is done).
ChangeDetection.ChildPropertyChange – This would mean either the object reference of the parameter has changed or any of its child properties have changed. To be used when having a view-model-like structure to be rendered as immutable children. (Like the initial example provided in this issue).
ChangeDetection.TrackChangeProperties – This is the same as previous but while the previous compares all child properties this would use the properties you have specified in a TrackChange attribute.
ChangeDetection.UsingCustomEquals – When there is an overridden Equals you want to use. Could be used when having an entity version (from rowversion in db) you want to use.

Implementation discussions/speculation: 😊
Perhaps one could introduce another frame type for these new change detection options and test accordingly. One way to achieve tests on child properties of a type is to flatten them (first level) into attribute-like properties in the render-tree. They will then be tested for change and could be hidden when setting properties to the component. Simulated in this fiddle: https://blazorfiddle.com/s/7tfa2wh1
I think the most reasonable default on a child property of reference type is to do a reference-test and see the children as immutable. (The always render option wouldn’t make sense obviously). In case you want another setting one could have another attribute to specify it, like:
[ChangeDetectionOnChild(nameof(..property..), ChangeDetection.OnInitialization)

About the TrackChangeProperties option: Sometimes you only want to display some part of an entity, and instead of mapping everything one could have additional attributes to specify them. Simulated in this fiddle: https://blazorfiddle.com/s/eq9qwspc

About delegates and OnInitialization option: I believe that the builder could have knowledge that it is in the first render so the compiler could generate a guard for delegate construction also, like builder.AddAttribute(seq, builder.IsInitialRender ? () => MyMethod() : null), which should give some performance boost and less memory foot-print.

About template parameters: template context parameters are usually also of reference type so it feels like it would make sense to have the ability setting this change-detection-option on the template parameter tag also (perhaps in combination of ability to specify a selector).

We will look into this to see how we can improve this area to make it simpler to use in the near future.

@mkArtakMSFT I am glad to hear. Is it something along this line? Any info/plan you can share?

[harleydk]

If only we could reference individual components and call Render on them by our own discretion, that's really all it would take. Overload the ShouldRender to return false, take control of the render for any given scenario where we feel the out-of-the-box wouldn't work to our advantage. That's the kind of flexibility and control I want from a framework such as this.

[Liander]

@pranavkm OK, I will chime in here too 😊. We need the ability to make components encapsulated, i.e. have render reactions independent on its placement on a page!

I think the order is to solve the general encapsulation ability first and then make use of it in situation reported in this issue.

Step 1) Encapsulated components: The problem today is that the framework has the pseudo-code (simplified) of

bool TestHasChanged(component-tree) 
{
  for-all attribute-values 
  {
    if (!IsKnownImmutableType(newValue.GetType()) return true; // <-- This is what kills component encapsulation
    if (newValue != oldValue) return true;
  }
  return false; // all equal
}

and the suggestion in #21915 is to have the _basically the same code_ but in different places

AddAttribute<T, TCompare>(int seq, string name, T value, TCompare renderCompareValue = null)
{ 
  // Now this default is overridable in the builder API
  renderCompareValue ??= (IsKnownImmutableType(typeof(T)) ? value : RenderOption.Reset);  
  // (Reset would be an object having Equals always returning false, which is the default
  // for not-known-immutable-types.)
  ...
} 
bool TestHasChanged(component-tree) 
{
  for-all attribute-values {
    // no special conditions here in this version
    if (newCompareValue != oldCompareValue) return true;
  }
  return false;
}

This removes the line that kills the encapsulated behavior, but instead it is set as default in the builder API which can be overridden when building components. Please observe that this could be delivered without any change in the current behavior today and without any change in tooling, and only to be used for manual construction for advanced scenarios to begin with. Basically the #21915 proposal.

Step 2) The second step is how the tooling should start to make use of it from razor files to make it user friendly. That is a much more involved discussion and depends on requirements within the tooling that I am not aware of. But let's say you are allowed to register projector-factories to transform an attribute value to a render-compare-value and let’s say you can have the syntax of AttributeName:ProjectorName=value with the result of generating AddAttribute(seq, AttributeName, value, ProjectorName.Project(value)) then a component could be given as

<WeatherForecast Data:Immutable=@forecast OnClick:OneTime=@OnClick Id:Self=@SomeGuid>
</WeatherForecast>

with the projectors of

• Immutable -> generated immutable clone (can be built up by RenderTreeFrames if you like...)
• OneTime -> true (or something that does not change)
• Self -> the value itself (for immutable types unknown to the framework, like Guid or immutable user object)
• Auto -> to inspect option on receiver (the component) instead, taken from metadata on component type. It will open up ability to have context specialized projection constructions, like a facade or a proxy-value from an object-version-property, etc.

When I say register, I mean it loosely because I do not know tooling constraints, but it could be like the helper function for bind that exists today but having it identified through attributes so they can become more configurable.

RenderFragment parameters are special in that they are specified as markup and that it is no information in its value (the RenderFragment) that can be transformed to its dependencies to render. For them it makes more sense to just specify the render-compare-value directly:

<WeatherReport>
  <Header [email protected]> 
    <p>Weather report</p>
  </Header>
  <Summary [email protected]>
    <p>@(forcast.TemperatureC > 20 ? "Nice today!" : "Just wait, earth is warming up...")</p>
  </Summary>
</WeatherReport>

And if you do not specify any of those you will always get the fallback currently present that is always safe but may break the encapsulation of the rendering decision.

@pranavkm One of the weirdest behavior currently is that an encapsulated component loose its encapsulation when declaring child content of static html. So basically, the test for change only works at leaf nodes with no interactions (between components) and with only primitive parameters! The comments in #21915 to focus on immutability really worries me, since you might be able to shoehorn in that without solving the underlying problem 😟. The step 2 also have alot more open questions. I am not sensitive for having another builder API than I proposed, but is there the understanding that references to objects of deep structure is just a special case of the fact that the attribute values themselves cannot and should not always determine the condition for rendering and the context where used may have more information of what that condition should be? I am also fine with handling it as a design sub-task but I really think this issue is best handled by resolving the generic encapsulation ability problem first as a standalone capability and then use that ability to solve it for references of deep structures if that is the most requested situation. Let me know if there is more to be explained...😊

[Marfusios]

Please get inspiration from Angular framework. They already iterated over a few solutions and converged to a decent approach.

Something like OnPush strategy would be great. It is an optional way to get full control over change detection - you have to change the object reference of input property (or raise output event) in order to trigger change detection.

More info:
https://blog.angular-university.io/how-does-angular-2-change-detection-really-work/

[endeffects]

@mkArtakMSFT

I run into the same issue today. I tried to prevent useless re-rendings within my child component,
but due the reason that the OnParametersSet Event does not provide any information
about the source of the change nor the modified parameters i'm unable to fix this in a smart way.

Maybe most issues with the behaviour of the OnParameterSet event could be solved
if you provide the type of the source component that triggered the update or atleast
an information that the update was triggered from the outside.

[mkArtakMSFT]

This is part of #22432