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

Nested business object depth exponentially increases the number of Root.OnPropertyChanged calls #813

Closed
keithdv opened this Issue Feb 16, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@keithdv
Contributor

keithdv commented Feb 16, 2018

This caused sever performance issues for us with a 3-Tier WPF application. The in-memory calculations took 30 seconds plus. So button click - UI blocked for 30+ seconds.

We dug and dug thinking it was the size of the business object graph or the number of calculations which are both large. However, removing chunks didn't create any substantial gains. Finally we realized our Root.OnPropertyChanged event was being raised 54 million times. Again, removing chucks didn't create any substantial gains. Further investigation we found the root cause is the depth of the object graph - up to 10 business objects. For each level down the number of Root.OnPropertyChange calls grows exponentially.

Example: https://github.com/keithdv/CslaNestedBO.git

Simple business object (SimpleBO) with a Name and Depth property and itself as a child property.
(class SimpleBO { string Name; int Depth; SimpleBO Child})

Count of Root.OnPropertyChanged for each update of SimpleBO.Name at each depth:

Depth: 0 PropertyChangedCount: 7
Depth: 1 PropertyChangedCount: 21
Depth: 2 PropertyChangedCount: 84
Depth: 3 PropertyChangedCount: 336
Depth: 4 PropertyChangedCount: 1344
Depth: 5 PropertyChangedCount: 5376
Depth: 6 PropertyChangedCount: 21504
Depth: 7 PropertyChangedCount: 86016
Depth: 8 PropertyChangedCount: 344064
Depth: 9 PropertyChangedCount: 1376256
Depth: 10 PropertyChangedCount: 5505024
Depth: 11 PropertyChangedCount: 22,020,096
Depth: 12 PropertyChangedCount: 88,080,384
Depth: 13 PropertyChangedCount: 352,321,536

It jumps into the billions after this.
Root cause is BusinessBase.OnChildChanged. This causes each level to create a "zig-zag" up and down the graph.

[EditorBrowsable(EditorBrowsableState.Advanced)]
protected virtual void OnChildChanged(ChildChangedEventArgs e)
{
  if (_childChangedHandlers != null)
    _childChangedHandlers.Invoke(this, e);
  MetaPropertyHasChanged("IsDirty");
  MetaPropertyHasChanged("IsValid");
  MetaPropertyHasChanged("IsSavable");
}

So, now the hard part. What's the solution? My guess is this is for when the UI binds to one of the meta-state properties on a mid-level business object. Anything more?

One solution would be to not use INotifyPropertyChanged.OnPropertyChanged to communicate between business objects. That way INotifyPropertyChanged.OnPropertyChanged could be called on each BO as we traverse up the object graph BUT this would not lead to a "zig-zag" up and down the graph. However, there's fair amount of code. Does this make sense? Thoughts? Better ideas (I hope)??

@keithdv

This comment has been minimized.

Show comment
Hide comment
@keithdv

keithdv Feb 16, 2018

Contributor

I'll add that after overriding MetaPropertyHasChanged to bypass the logic during the button click the operation takes a second or two (versus 30 sec+).

Contributor

keithdv commented Feb 16, 2018

I'll add that after overriding MetaPropertyHasChanged to bypass the logic during the button click the operation takes a second or two (versus 30 sec+).

@jonnybee

This comment has been minimized.

Show comment
Hide comment
@jonnybee

jonnybee Feb 16, 2018

Member

There is a thread from the old forum about this issue:
http://cslanet.com/old-forum/11835.html

Member

jonnybee commented Feb 16, 2018

There is a thread from the old forum about this issue:
http://cslanet.com/old-forum/11835.html

@keithdv

This comment has been minimized.

Show comment
Hide comment
@keithdv

keithdv Feb 16, 2018

Contributor

So let's fix it!

Contributor

keithdv commented Feb 16, 2018

So let's fix it!

@keithdv

This comment has been minimized.

Show comment
Hide comment
@keithdv

keithdv Feb 17, 2018

Contributor

Ok, I have a fix in place. Moved everything to ChildChanged. OnPropertyChanged is now only responsible for screen bindings. See the following pull request:

#814

Results:

Depth: 0 PropertyChangedCount: 10
Depth: 1 PropertyChangedCount: 3
Depth: 2 PropertyChangedCount: 3
Depth: 3 PropertyChangedCount: 3
Depth: 4 PropertyChangedCount: 3
Depth: 5 PropertyChangedCount: 3
Depth: 6 PropertyChangedCount: 3
Depth: 7 PropertyChangedCount: 3
Depth: 8 PropertyChangedCount: 3
Depth: 9 PropertyChangedCount: 3
Depth: 10 PropertyChangedCount: 3
Depth: 11 PropertyChangedCount: 3
Depth: 12 PropertyChangedCount: 3
Depth: 13 PropertyChangedCount: 3
Done

Contributor

keithdv commented Feb 17, 2018

Ok, I have a fix in place. Moved everything to ChildChanged. OnPropertyChanged is now only responsible for screen bindings. See the following pull request:

#814

Results:

Depth: 0 PropertyChangedCount: 10
Depth: 1 PropertyChangedCount: 3
Depth: 2 PropertyChangedCount: 3
Depth: 3 PropertyChangedCount: 3
Depth: 4 PropertyChangedCount: 3
Depth: 5 PropertyChangedCount: 3
Depth: 6 PropertyChangedCount: 3
Depth: 7 PropertyChangedCount: 3
Depth: 8 PropertyChangedCount: 3
Depth: 9 PropertyChangedCount: 3
Depth: 10 PropertyChangedCount: 3
Depth: 11 PropertyChangedCount: 3
Depth: 12 PropertyChangedCount: 3
Depth: 13 PropertyChangedCount: 3
Done

@keithdv

This comment has been minimized.

Show comment
Hide comment
@keithdv

keithdv Feb 19, 2018

Contributor

Just wanted to note the final approach is NOT "Moved everything to ChildChanged. OnPropertyChanged is now only responsible for screen bindings."

Rather parent business objects continue to listen to Child.INotifyPropertyChanged.OnPropertyChanged. Only created MetaPropertyChangedEventArgs which inherits from PropertyChangedEventArgs and is raised by MetaPropertyHasChanged. This way parent business objects can distinguish the problematic "MetaPropertyHasChanged" calls and stop the snowball effect.

Contributor

keithdv commented Feb 19, 2018

Just wanted to note the final approach is NOT "Moved everything to ChildChanged. OnPropertyChanged is now only responsible for screen bindings."

Rather parent business objects continue to listen to Child.INotifyPropertyChanged.OnPropertyChanged. Only created MetaPropertyChangedEventArgs which inherits from PropertyChangedEventArgs and is raised by MetaPropertyHasChanged. This way parent business objects can distinguish the problematic "MetaPropertyHasChanged" calls and stop the snowball effect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment