Community
Participate
Working Groups
Here is a list of trails (see bug 445802 comment #79): 1) remove additional thread for notification sending to listeners (use UI thread if listeners don't take longer than about 6ms). Otherwise, keep the additional thread 2) remove all the synchronized blocks, Concurrent data structures, and asyncExecs from those listeners (we can already do that without (1) as they are now called from a single thread) 3) find a way to remove the 'jobs' map from ProgressManager, e.g. by attaching JobInfo to the Job somehow and only access it from the job's thread itself.
Regarding 3, we may be able to accomplish this by putting any data structures we need right in the progress monitor, which is attached to the job and only invoked from the job's thread.
Regarding 2), I'm not sure you can remove those synchronized blocks yet since we also use a lot of those structures in the UI thread.
(In reply to Stefan Xenos from comment #1) > Regarding 3, we may be able to accomplish this by putting any data > structures we need right in the progress monitor, which is attached to the > job and only invoked from the job's thread. yep, or I was thinking about Job.setProperty(). Any suggestion about that? (In reply to Stefan Xenos from comment #2) > Regarding 2), I'm not sure you can remove those synchronized blocks yet > since we also use a lot of those structures in the UI thread. You're right. I've made an initial test: I invoked everything from the UI thread, and blindly removed all the synchronize blocks and async calls. Even without throttling, the progress reporting is much smoother (looks like ProgressMonitorDialog, without sluggish progress). It does not perform as well as the current version with throttling but I'm confident we can get there with a bit of profiling...
> It does not perform as well as the current version with Hmm. That would be a very strong argument against my suggestion. IMO, we should do what's fastest. Could I trouble you to attach your test case here so I can profile it, too?
Created attachment 263879 [details] Heavy progress reporting: differences between fixes for bugs 44502 and 500332 I've managed to do more testing last evening. Using the UI thread (and removing the necessary synchronization) to notify listeners does not provide better performance, but it let us have a *much* smoother reporting. Look at the attached video and look at how the progress are reported on the bottom screen compared to the others. The patch still needs a bit of work, but I plan to submit it by next Monday.
From the video it looks like the smoother progress may be a consequence of posting more frequent UI updates, which may explain the why the performance didn't change. Although it certainly looks a lot smoother, I'd like to review the patch before submission to be sure that we're not creating any UI potential for UI freezes.
I've pushed https://git.eclipse.org/r/#/c/80386/ for early review. This patch does several things that I may need to split into several commits: - Use the UI thread to notify ProgressManager (PM) listeners - Remove synchronized blocks / concurrent data structures from listeners - Remove the unnecessary workbench jobs for refresh (and the concept of infra job at the same time) - Move the (debug / show all jobs) boolean from the listeners to the PM. Please, feel free to comment here or on the patch. Thanks.
New Gerrit change created: https://git.eclipse.org/r/82283
I've updated https://git.eclipse.org/r/#/c/80386/ and split the storage of JobInfo in https://git.eclipse.org/r/82283. I now have performances equivalent between ProgressMonitorDialog and standard Jobs (with bug 445802 comment 1 examples and the benchmarks you've added).
(I'm looking at the failed tests with these latest change sets)
The failing test was assuming everything was done synchronously in the UI thread. With my patch, the listeners are notified with an asyncExec, so we need to queue the assert in the async queue (this way, the listeners will be notified before the assert is executed). If the build of patch set 3 is green, I consider these patches good for review :) Thanks.
Gerrit change https://git.eclipse.org/r/80386 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=cd6a4cceaccf7c8792eeb5b1d70e1f5005e2e554
Note that this causes problems with Oomph. https://bugs.eclipse.org/bugs/show_bug.cgi?id=508982 Please do not make the platform even harder to reuse without a Workbench instance. We already have to jump through hoops in way too many places that make such bad assumptions. E.g., the JobManager or simple filter tree's refresh job. Obviously we need to use the JobManager in Oomph because p2 uses and it already works poorly without a workbench, which is the reason for this code being there. So please please don't make it even worse.
The workbench dependency has been introduced by the usage of PlatformUI#getWorkbench()#getDisplay(). While Display#getDefault() would return the same result in most cases, I can't help to think that depending on the workbench is not inherently wrong from within an internal package in org.eclipse.ui.*workbench* plugin :) Moreover, it is specifically advised in the javadoc to use IWorbench#getDisplay() instead of Display#getDefault()... I will provide a patch shortly to use Display static factory instead of IWorkbench#getDisplay().
I've submitted https://git.eclipse.org/r/86853
Failures from the latest build: Regression org.eclipse.e4.ui.tests.workbench.PartFocusTest.testFocusChangesOnExplicitPartActivation (from org.eclipse.e4.ui.tests.UIAllTests) java.lang.AssertionError: null at org.junit.Assert.fail(Assert.java:86) at org.junit.Assert.assertTrue(Assert.java:41) at org.junit.Assert.assertTrue(Assert.java:52) at org.eclipse.e4.ui.tests.workbench.PartFocusTest.setUp(PartFocusTest.java:134) org.eclipse.e4.ui.tests.workbench.PartFocusTest.testNoActivationOnExplicitInPartWidgetSelection (from org.eclipse.e4.ui.tests.UIAllTests) java.lang.AssertionError: null at org.junit.Assert.fail(Assert.java:86) at org.junit.Assert.assertTrue(Assert.java:41) at org.junit.Assert.assertTrue(Assert.java:52) at org.eclipse.e4.ui.tests.workbench.PartFocusTest.setUp(PartFocusTest.java:134)
Stefan, I don't see the relationship between this bug and test failures from the latest build. Could you please elaborate? Thanks.
> Stefan, I don't see the relationship between this bug and test > failures from the latest build. Neither do I, but I can't give this patch a +2 when it has a long string of failed builds in its recent history. If you can get gerrit to produce a successful build from it, I'll review it.
Sorry for the misunderstanding. I thought you were talking about test failures in the latest nightly build of Eclipse (http://download.eclipse.org/eclipse/downloads/) (facepalm). You were referring to https://git.eclipse.org/r/#/c/82283/. I agree about not integrating this patch until the build is more stable. I will investigate this further (along with bug 506562).
IMO, latest patchset (12) fixes the issue. I've changed the way we keep track of Job managed by the ProgressManager. Previously, it was done by checking whether a JobInfo had been created and put in a Map. This patch forces the creation of JobInfo when the JobMonitor is created. I now only keep a map of managed jobs and JobInfo are always retrieved from the JobMonitor. The two failures happened in other gerrit builds as well (e.g. https://hudson.eclipse.org/platform/job/eclipse.platform.ui-Gerrit/11607/) so likely unrelated.
(In reply to Mikaël Barbero from comment #20) > The two failures happened in other gerrit builds as well (e.g. > https://hudson.eclipse.org/platform/job/eclipse.platform.ui-Gerrit/11607/) > so likely unrelated. I confirm. The two test failures are unrelated (see bug 505678 comment 13)
Gerrit change https://git.eclipse.org/r/82283 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=68ecfe90c6fc3d47f58d01b125cff47de7634954
I think this is fixed now, or were there still some follow-ups to address?
I think we can consider this as fixed. If any other performance issue is identified, I suggest we track it in another bug. Thanks for your help Stefan.
