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

BindableCollection dispatches operations on the collection to the UI thread - why not just the change events? #247

Closed
cdanne opened this Issue Nov 12, 2015 · 15 comments

Comments

Projects
None yet
5 participants
@cdanne

cdanne commented Nov 12, 2015

Hi there,

we have a scenario where a BindableCollection is not only modified by user interaction (drag & drop of list items), but also may get many updates in short time from a background operation. In the latter case, performance is an issue and we set IsNotifying=false before the algorithm starts and re-enable change events afterwards.

That improved performance because it removed UI updates, but didn't make it great... We profiled the app and realized that a major performance killer is the fact that all operations on the collection (Add, Remove etc.) are dispatched to the UI thread via Execute.OnUIThread(...) in the BindableCollection implementation.

Question: Why so? Isn't it enough to have the overwritten OnCollectionChanged() and OnPropertyChanged() raise the change event on the UI thread?
So there would be no waiting for the UI thread to modify the collections in the first place. Dispatching to the UI thread only happens for the change events and that only in case IsNotifying is true.

We did such an implementation and it performs better and seems to work OK in our scenario. Are there any pitfalls with that approach which we oversee or would such an implementation be equally "correct" and faster in such scenarios?

@cdanne cdanne changed the title from BindableCollection dispatches all operations on the collection to the UI thread - why not just the change event? to BindableCollection dispatches operations on the collection to the UI thread - why not just the change event? Nov 12, 2015

@cdanne cdanne changed the title from BindableCollection dispatches operations on the collection to the UI thread - why not just the change event? to BindableCollection dispatches operations on the collection to the UI thread - why not just the change events? Nov 12, 2015

@nigel-sampson

This comment has been minimized.

Show comment
Hide comment
@nigel-sampson

nigel-sampson Nov 15, 2015

Contributor

This code predates my taking over the project so I can't speak to the logic of why almost all the operations were wrapped in Execute.OnUIThead. On review it does look overzealous.

I'd love to see a pull request with your implementation so we can discuss it further.

On a side note the implementation of XamlPlatformProvider.OnUIThread checks for access before using the dispatcher. So if you have a method on a background thread doing a lot of small updates then shifting the whole method over should give you some performance improvements as well.

Contributor

nigel-sampson commented Nov 15, 2015

This code predates my taking over the project so I can't speak to the logic of why almost all the operations were wrapped in Execute.OnUIThead. On review it does look overzealous.

I'd love to see a pull request with your implementation so we can discuss it further.

On a side note the implementation of XamlPlatformProvider.OnUIThread checks for access before using the dispatcher. So if you have a method on a background thread doing a lot of small updates then shifting the whole method over should give you some performance improvements as well.

@rohits79

This comment has been minimized.

Show comment
Hide comment
@rohits79

rohits79 May 28, 2016

As a work around: Reactive Extension can be used here to throttle or buffer updates

As a work around: Reactive Extension can be used here to throttle or buffer updates

@nigel-sampson nigel-sampson added this to the v3.1.0 milestone May 28, 2016

@jongleur1983

This comment has been minimized.

Show comment
Hide comment
@jongleur1983

jongleur1983 Oct 5, 2016

does anyone have any idea how to write tests for this change?
Took the code we have at office (as @cdanne proposed) and changed the BindableCollection accordingly.
All existing caliburn unit tests still running, tested in real-live code at work, but not covered by test code yet.
Is there any way to write unit tests that properly test thread dispatching (on UI thread, ideally without UI)?

does anyone have any idea how to write tests for this change?
Took the code we have at office (as @cdanne proposed) and changed the BindableCollection accordingly.
All existing caliburn unit tests still running, tested in real-live code at work, but not covered by test code yet.
Is there any way to write unit tests that properly test thread dispatching (on UI thread, ideally without UI)?

@nigel-sampson

This comment has been minimized.

Show comment
Hide comment
@nigel-sampson

nigel-sampson Oct 5, 2016

Contributor

You could potentially test this by creating a fake platform provider that recorded calls to dispatch on the UI thread.

Your test could then do a number of operations and then assert the amount of ui thread dispatches was correct.

Contributor

nigel-sampson commented Oct 5, 2016

You could potentially test this by creating a fake platform provider that recorded calls to dispatch on the UI thread.

Your test could then do a number of operations and then assert the amount of ui thread dispatches was correct.

jongleur1983 added a commit to jongleur1983/Caliburn.Micro that referenced this issue Oct 5, 2016

#247: do less work on the UI thread in BindableCollection
see ticket and tickets discussion for rationale.
Tests missing yet (have to figure out how to proper test with thread dispatching).
@jongleur1983

This comment has been minimized.

Show comment
Hide comment
@jongleur1983

jongleur1983 Oct 9, 2016

I guess the fix up to here doesn't solve the problem but just flips it around:
The original Caliburn implementation did any manipulation of the BindableCollection on the UI-Thread.
The issue raised here is, that that's a bad idea because throughput usually is a critical issue on the UI-Thread: The work to be done on the UI should be restricted as far as possible.

The commit I sent before "fixes" this by doing anything except the Update-Notifications off the UI-Thread.
Accidently this breaks the side-effect of running everything on the UI-Thread, that everything runs on the same thread, thus avoiding race conditions when the collection is manipulated from different threads concurrently.

A better solution would be to create a specific thread to do the collection changes on.
This would work as follows:

The BindableCollection would take a distinct Manipulator-Thread. This Thread get's it's own Dispatcher.
Then that Manipulator-Dispatcher is used to dispatch any operation changing the BindableCollection, while the default dispatcher of WPF, which is bound to the UI-Thread, is used for the Change-Notifications.

This way I hope to solve both concerns, the one raised by @cdanne and the one solved by the original solution to always use the UI-Thread.

I guess the fix up to here doesn't solve the problem but just flips it around:
The original Caliburn implementation did any manipulation of the BindableCollection on the UI-Thread.
The issue raised here is, that that's a bad idea because throughput usually is a critical issue on the UI-Thread: The work to be done on the UI should be restricted as far as possible.

The commit I sent before "fixes" this by doing anything except the Update-Notifications off the UI-Thread.
Accidently this breaks the side-effect of running everything on the UI-Thread, that everything runs on the same thread, thus avoiding race conditions when the collection is manipulated from different threads concurrently.

A better solution would be to create a specific thread to do the collection changes on.
This would work as follows:

The BindableCollection would take a distinct Manipulator-Thread. This Thread get's it's own Dispatcher.
Then that Manipulator-Dispatcher is used to dispatch any operation changing the BindableCollection, while the default dispatcher of WPF, which is bound to the UI-Thread, is used for the Change-Notifications.

This way I hope to solve both concerns, the one raised by @cdanne and the one solved by the original solution to always use the UI-Thread.

@jongleur1983

This comment has been minimized.

Show comment
Hide comment
@jongleur1983

jongleur1983 Oct 10, 2016

after discussion with @TeaDrivenDev and others today I came to the following concept to solve this issue:

The goal is to avoid (blocking) work on the UI thread, while keeping the collection manipulations thread safe.
One way to keep thread safety is to do all of the critical work on the same thread. This is the current approach without any changes, but it uses the UI thread as that one thread.

My approach would be to use the same approach, but a different, dedicated thread to process the work.
For WPF on .NET 4.5 alone this would be easy (spawn a thread, get it's dispatcher, use the thread as a dispatcher-thread and dispatch work on it).

It get's complicated when the other platforms are introduced, as there are three different Dispatcher implementations to target.

What i have locally yet consists of an extension to the IPlatformProvider by a method to get a Dispatcher d of a newly spawned thread t.

The BindableCollection then can get this dispatcher - and with it a dedicated thread that sleeps most of the time as long as nothing happens on the Collection.
Whenever the collection is manipulated the critical part of the code is dispatched to t serializing the manipulations.

My current problem is to get some kind of unified Dispatcher object with a unified API across the platforms that can be used for this.

Missing parts yet:

  • Windows.UI.Core.CoreDispatcher: cannot be instantiated, as far as I understand the docs there's only one single Dispatcher here.
  • different interfaces (without any common part) between Windows.UI.Core.CoreDispatcher and System.Windows.Threading.Dispatcher

after discussion with @TeaDrivenDev and others today I came to the following concept to solve this issue:

The goal is to avoid (blocking) work on the UI thread, while keeping the collection manipulations thread safe.
One way to keep thread safety is to do all of the critical work on the same thread. This is the current approach without any changes, but it uses the UI thread as that one thread.

My approach would be to use the same approach, but a different, dedicated thread to process the work.
For WPF on .NET 4.5 alone this would be easy (spawn a thread, get it's dispatcher, use the thread as a dispatcher-thread and dispatch work on it).

It get's complicated when the other platforms are introduced, as there are three different Dispatcher implementations to target.

What i have locally yet consists of an extension to the IPlatformProvider by a method to get a Dispatcher d of a newly spawned thread t.

The BindableCollection then can get this dispatcher - and with it a dedicated thread that sleeps most of the time as long as nothing happens on the Collection.
Whenever the collection is manipulated the critical part of the code is dispatched to t serializing the manipulations.

My current problem is to get some kind of unified Dispatcher object with a unified API across the platforms that can be used for this.

Missing parts yet:

  • Windows.UI.Core.CoreDispatcher: cannot be instantiated, as far as I understand the docs there's only one single Dispatcher here.
  • different interfaces (without any common part) between Windows.UI.Core.CoreDispatcher and System.Windows.Threading.Dispatcher
@nigel-sampson

This comment has been minimized.

Show comment
Hide comment
@nigel-sampson

nigel-sampson Oct 11, 2016

Contributor

This feels like a lot of work for what should be a simple collection. The only real reason it exists is because a lot of the implementations of ObservableCollection<T>` didn't dispatch their change notifications onto the UI thread.

The added complications of other platforms just get terrible.

Right now the bug issue is we're dispatching in a chatty way, rather than a chunky way, It may be best to keep the solution to just that.

Making it a completely thread safe collection while doing this can be a separate piece of work (with an associated discussion on whether it's necessary / better way to do it).

Contributor

nigel-sampson commented Oct 11, 2016

This feels like a lot of work for what should be a simple collection. The only real reason it exists is because a lot of the implementations of ObservableCollection<T>` didn't dispatch their change notifications onto the UI thread.

The added complications of other platforms just get terrible.

Right now the bug issue is we're dispatching in a chatty way, rather than a chunky way, It may be best to keep the solution to just that.

Making it a completely thread safe collection while doing this can be a separate piece of work (with an associated discussion on whether it's necessary / better way to do it).

@jongleur1983

This comment has been minimized.

Show comment
Hide comment
@jongleur1983

jongleur1983 Oct 11, 2016

@nigel-sampson thanks for the comment, but I oppose: By the initial commit we don't "keep the solution to just that", but we introduce possible bugs that were not possible before.

But Sometimes having a good night isn't a bad thing, and talking to others isn't either.

Basically we came to the conclusion that the Dispatching stuff isn't even necessary to be thread save. All we want is not losing the concurrency-safety the BindableCollection had before (as that runs the risk to break existing code using the BindableCollection) while reducing the UI-Thread work.

By a simple lock we achieve the same goal across threads: A single lock (per collection instance) is used wrapping all manipulation commands of the base implementation.
Like dispatching to the UI-Thread did before the locking linearizes execution of the critical source.

As this locking is done on a single level only (by wrapping the base implementation), and there is only one lock object involved, it should be impossible to get a deadlock (deadlock always needs two interleaving locks).

Unit tests still working, but I want to do some more testing tomorrow on real-life code.

Nevertheless I appreciate any comments and remarks (except for the typo in the issue number in the commit message - realized that myself).

jongleur1983 commented Oct 11, 2016

@nigel-sampson thanks for the comment, but I oppose: By the initial commit we don't "keep the solution to just that", but we introduce possible bugs that were not possible before.

But Sometimes having a good night isn't a bad thing, and talking to others isn't either.

Basically we came to the conclusion that the Dispatching stuff isn't even necessary to be thread save. All we want is not losing the concurrency-safety the BindableCollection had before (as that runs the risk to break existing code using the BindableCollection) while reducing the UI-Thread work.

By a simple lock we achieve the same goal across threads: A single lock (per collection instance) is used wrapping all manipulation commands of the base implementation.
Like dispatching to the UI-Thread did before the locking linearizes execution of the critical source.

As this locking is done on a single level only (by wrapping the base implementation), and there is only one lock object involved, it should be impossible to get a deadlock (deadlock always needs two interleaving locks).

Unit tests still working, but I want to do some more testing tomorrow on real-life code.

Nevertheless I appreciate any comments and remarks (except for the typo in the issue number in the commit message - realized that myself).

jongleur1983 referenced this issue in jongleur1983/Caliburn.Micro Oct 11, 2016

#274: solve regression problem with concurrent manipulations
see Issue thread for more details, but the lock on the manipulationLock field prevents concurrent modification of the collection now, where dispatching to the UI-Thread and by that accidentally linearizing these manipulations did the same.

@nigel-sampson nigel-sampson modified the milestones: v4.0.0, v3.1.0 May 18, 2017

@nigel-sampson

This comment has been minimized.

Show comment
Hide comment
@nigel-sampson

nigel-sampson May 18, 2017

Contributor

Looking over this some more with the view to release 3.1.0.

I want to tackle this a different way. The first is something akin to what got brought up in #407 as a way to opt out of both BindableCollection<T> and PropertyChangedBase doing UI thread dispatching.

I think as evidenced here different people have different goals and opinions on what they want, especially when it comes to high performance updates.

What I propose is adding a new virtual method that will handle dispatching on both BindableCollection<T> and PropertyChangedBase and replacing existing uses of Execute.OnUIThread with this.

protected virtual void OnUIThread(System.Action action) => action.OnUIThread();

This should allow customization in most scenarios to what people want to do, such as add a boolean property to turn it off at times, or there own custom thread dispatching.

Then in 4.0.0 we can investigate the possibility of paring back the usages of this method to only on event notifications. I'd like to do this separately as the second part has the potential for breaking changes.

Thoughts?

Contributor

nigel-sampson commented May 18, 2017

Looking over this some more with the view to release 3.1.0.

I want to tackle this a different way. The first is something akin to what got brought up in #407 as a way to opt out of both BindableCollection<T> and PropertyChangedBase doing UI thread dispatching.

I think as evidenced here different people have different goals and opinions on what they want, especially when it comes to high performance updates.

What I propose is adding a new virtual method that will handle dispatching on both BindableCollection<T> and PropertyChangedBase and replacing existing uses of Execute.OnUIThread with this.

protected virtual void OnUIThread(System.Action action) => action.OnUIThread();

This should allow customization in most scenarios to what people want to do, such as add a boolean property to turn it off at times, or there own custom thread dispatching.

Then in 4.0.0 we can investigate the possibility of paring back the usages of this method to only on event notifications. I'd like to do this separately as the second part has the potential for breaking changes.

Thoughts?

nigel-sampson added a commit that referenced this issue May 22, 2017

#247 Add UI thread customisation
To BindableCollection.

nigel-sampson added a commit that referenced this issue May 22, 2017

#247 Add UI Thread Customation
To PropertyChangedBase
@superware

This comment has been minimized.

Show comment
Hide comment
@superware

superware Jun 12, 2017

@nigel-sampson but how can this be used for all built-in PropertyChangedBase inheriting classes?

@nigel-sampson but how can this be used for all built-in PropertyChangedBase inheriting classes?

@nigel-sampson

This comment has been minimized.

Show comment
Hide comment
@nigel-sampson

nigel-sampson Jun 12, 2017

Contributor

The commit 6a8d29d adds the same behavior to PropertyChangedBase. You should be able to override OnUIThread in those classes and do your own dispatching.

Contributor

nigel-sampson commented Jun 12, 2017

The commit 6a8d29d adds the same behavior to PropertyChangedBase. You should be able to override OnUIThread in those classes and do your own dispatching.

@superware

This comment has been minimized.

Show comment
Hide comment
@superware

superware Jul 18, 2017

Hi @nigel-sampson,

Are you suggesting to subclass PropertyChangedBase, Screen, Conductor etc and override OnUIThread for each one manually? If so it sounds very cumbersome to say the least.

IMHO this should be controlled centrally and affect all existing relevant composition classes (simply switch this marshaling behavior off), see you comment.

Hi @nigel-sampson,

Are you suggesting to subclass PropertyChangedBase, Screen, Conductor etc and override OnUIThread for each one manually? If so it sounds very cumbersome to say the least.

IMHO this should be controlled centrally and affect all existing relevant composition classes (simply switch this marshaling behavior off), see you comment.

@nigel-sampson

This comment has been minimized.

Show comment
Hide comment
@nigel-sampson

nigel-sampson Jul 18, 2017

Contributor

I'm not totally convinced it should be global. I've seen some scenarios where it being on an instance basis is better.

If you truly want to disable it, you could use a custom platform provider to stop all marshalling to the UI thread.

Contributor

nigel-sampson commented Jul 18, 2017

I'm not totally convinced it should be global. I've seen some scenarios where it being on an instance basis is better.

If you truly want to disable it, you could use a custom platform provider to stop all marshalling to the UI thread.

@superware

This comment has been minimized.

Show comment
Hide comment
@superware

superware Jul 19, 2017

I have tens of view models deriving from PropertyChangedBase, Screen, Conductor etc, and I simply do not wish that all property changes will be marshaled to the UI thread.

IMHO it doesn't make sense to manually override OnUIThread per VM, and also not to create NotUIMarshaledPropertyChangedBase, NotUIMarshaledScreen etc.

WPF is handling property changes internally, please allow a way to disable this globally, since the default will be UI marshaling enabled, this won't be a breaking change.

"I would consider adding it to IPlatformProvider since it's static and then be configured solution wide." sounds like a perfect solution.

Thank you for everything.

superware commented Jul 19, 2017

I have tens of view models deriving from PropertyChangedBase, Screen, Conductor etc, and I simply do not wish that all property changes will be marshaled to the UI thread.

IMHO it doesn't make sense to manually override OnUIThread per VM, and also not to create NotUIMarshaledPropertyChangedBase, NotUIMarshaledScreen etc.

WPF is handling property changes internally, please allow a way to disable this globally, since the default will be UI marshaling enabled, this won't be a breaking change.

"I would consider adding it to IPlatformProvider since it's static and then be configured solution wide." sounds like a perfect solution.

Thank you for everything.

@nigel-sampson

This comment has been minimized.

Show comment
Hide comment
@nigel-sampson

nigel-sampson Jul 19, 2017

Contributor

If you're looking for sweeping changes I'd recommend creating a subclass of BindableCollection<T> or whichever one you need changing and inherit from that instead,

Contributor

nigel-sampson commented Jul 19, 2017

If you're looking for sweeping changes I'd recommend creating a subclass of BindableCollection<T> or whichever one you need changing and inherit from that instead,

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