Too much too many

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Too much too many

Tom Schindl-2
Hi,

In the past few weeks I had the fun to investigate performance problems
in an 3.x RCP application currently ported to e4 and the compat layer [1].

I'd say the figures are shocking and amazing at the same moment!

I've attached a gerrit-review with a strategy to queue updates (instead
of recalculating the same information in the same UI-Pass) by scheduling
them on an Display#asyncExec. I'm not totally happy with this strategy
but I could not come up with a better one.

You can play around yourself with turning on/off certain parts of the
fix using system-properites certainly the biggest impact has the
enabledWhen queueing.

The enabledWhen-Queueing is getting even better if you make the
EvaluationService sending the reevaluation command synchronously. I
don't know why this was done that way but it makes totally no sense to
post the event just to make the event-system to schedule an asyncExec
polluting the event-loop with runnables.

While looking into this part I looked at EventBroker and what I think
has not been a very good idea there was that all subscriptions are
forced back to the UI-Thread and the user of the API has no chance to
overrule that :-(

As of today @EventTopic is the only way to receive events outside the
UI-Thread which is very unfortunate (you all probably know that I think
@EventTopic and @UIEventTopic have been a bad bad idea) but it is not
used in the (platform) codebase.

I think Platform-Developers should keep that in mind when deciding
whether IEventBroker post/send should be used (99% I think you should
opt for send, if some handler is doing bad things the event-loop would
be stuck anyway by EventBroker and/or @UIEventTopic).

The root cause of all those problems is that we are
activating/reactivating too many IEclipseContext branches leading to a
flush of RAT-Recalcuations but addressing these problems is a full-time
job for the next months or so and nothing I can allocate funding for.

I'm writing down those findings because I'm 100% certain that there are
probably other code parts get triggered without a need.

Tom

[1]https://bugs.eclipse.org/bugs/show_bug.cgi?id=514277

--
Thomas Schindl, CTO
BestSolution.at EDV Systemhaus GmbH
Eduard-Bodem-Gasse 5-7, A-6020 Innsbruck
http://www.bestsolution.at/
Reg. Nr. FN 222302s am Firmenbuchgericht Innsbruck
_______________________________________________
platform-ui-dev mailing list
[hidden email]
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/platform-ui-dev
Reply | Threaded
Open this post in threaded view
|

Re: Too much too many

Stefan Xenos-3
To respond to some of your comments.

Yes, the use of synchronous events is a design flaw in the workbench, due to the event storms they create. This is a well known problem of using the observer pattern with synchronous events (see the comment about the repeated diamond pattern here: https://en.wikipedia.org/wiki/Reactive_programming#Evaluation_models_of_reactive_programming ).

Doing things asynchronously is much better. Although events sent via asyncExec is a good quick fix to speed things up in the short-term, it is still possible to create patterns of listeners that trigger event storms. A better approach (long term) would be to use an algorithm that dirties things that need to be recomputed and then recomputes them in the correct order without visiting any node more than once. I describe the algorithm in more detail here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=490149 - it would also work for everything that has listeners, not just databinding.

So I agree with the general approach suggested for your patch.

However, your message, above, also discusses permitting listeners to be notified on threads other than the UI thread, and this I strongly disagree with. Doing all your notifications on the UI thread is cheap as long as the listeners never do anything expensive synchronously, and they never need to. If a listener needs to do anything nontrivial, it can just mark something as dirty and initiate a job that does the expensive part. By allowing multithreaded notifications, you'd need to protect all the observable data and listener lists with mutexes or synchronized blocks, which are SLOW and destroy your cache utilization.

The current implementation of the Eclipse databinding library is a perfect example of why multithreaded listeners are a bad idea. It's full of data races and it has terrible overhead due to the (unnecessary) synchronization to support multithreaded listeners. Up until very recently, the progress monitor system also had severe performance problems due to its multithreaded notifications, and much of the speed-up came from moving stuff to the UI thread and removing the unnecessary synchronization.

Let's not conflate asynchronous notifications (which are a good thing) with multithreaded notifications (which are bad).

​  - Stefan

_______________________________________________
platform-ui-dev mailing list
[hidden email]
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/platform-ui-dev
Reply | Threaded
Open this post in threaded view
|

Re: Too much too many

Tom Schindl-2
[...]

> However, your message, above, also discusses permitting listeners to be
> notified on threads other than the UI thread, and this I strongly
> disagree with. Doing all your notifications on the UI thread is cheap as
> long as the listeners never do anything expensive synchronously, and
> they never need to. If a listener needs to do anything nontrivial, it
> can just mark something as dirty and initiate a job that does the
> expensive part. By allowing multithreaded notifications, you'd need to
> protect all the observable data and listener lists with mutexes or
> synchronized blocks, which are SLOW and destroy your cache utilization.

I didn't want to say that sending asynchronous-Events does not make
sense at all - there might be situations they make sense.

What I wanted to express was that (in the current setup) it makes
nosense at all to use IEventBroker#post() because in the end all what
happens is

1     Event is posted
1 - n Subscribers run code like this on their own

      Event evt = ...;
      Display.runAsyncExec( () -> process(evt) );

So posting 1 Event with with 10 subscribers leads to 10 scheduled
runnables on the event-queue.

I agree with you that we should have a central place where we collect
what we want to recalculate in the next EventLoop-Pass (this is how
SceneGraph-Toolkits like JavaFX work) and remove duplicate calculations
there, ... .

On the other hand we should try that we don't invalidate too much parts
(eg closing a window invalidates the complete IEclipseContext hierarchy!).

I'll cleanup the patch (I'll remove the synchronization stuff who just
can't happen, remove the parts who calculate the stats) and submit that
for review. I'd really like to this patch getting merged for M7 ASAP so
that we can see if that leads to any regressions.

Tom

--
Thomas Schindl, CTO
BestSolution.at EDV Systemhaus GmbH
Eduard-Bodem-Gasse 5-7, A-6020 Innsbruck
http://www.bestsolution.at/
Reg. Nr. FN 222302s am Firmenbuchgericht Innsbruck
_______________________________________________
platform-ui-dev mailing list
[hidden email]
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/platform-ui-dev