Community
Participate
Working Groups
Created attachment 269751 [details] Screenshot: Profiler When the progress monitor is updated a lot of times in a tight loop, a reasonable amount of time is spent updating it and slowing down UI. This effect can be observed for example when having a Git repo in the repositories view and use the context action "Import Projects". The import wizard will start scanning the git repository and the currently processed files are displayed. Attached profiler screenshot shows that ~2 sec is spent updating the label (reference repository used is lsp4e). Another place where ProgressMonitorPart.updateLabel() is when projects are deleted from the workspace. When executing the code org.eclipse.jface.wizard.ProgressMonitorPart.updateLabel() asynchronously the performance of the dialog improves reasonable. The "negative" effect is now that the progress label is updated so fast that no single entry could be read really.
New Gerrit change created: https://git.eclipse.org/r/102730
We've faced similar issues in bug 445802 and bug 500332. While you say that executing the update asynchronously improves the situation, I think that a better way to handle this issue is to introduce throttling (like we did in bug 500332). If you want, you can easily reause the throttler utility class (https://git.eclipse.org/c/platform/eclipse.platform.ui.git/tree/bundles/org.eclipse.ui.workbench/Eclipse%20UI/org/eclipse/ui/internal/progress/Throttler.java)
This is helpful and would be the right way to go, thanks for the pointer. But Throttler is in org.eclipse.ui.workbench and ProgressMonitor in org.eclipse.platform.ui. I think this would be an undesired dependency? And copying it? How about moving the class to platform.ui and make the dependency the other way around (o.e.ui.workbench depending on o.e.platform.ui, which isn't the case yet)? Please advise.
I guess the best solution would be to move the Throttler class to jface (maybe make it a public utility class in org.eclipse.jface.util?) and to change the usage in org.eclipse.ui.workbench. Is anyone against such a thing? I could do this change and then you could use Throttler from the ProgressMonitorPart.
New Gerrit change created: https://git.eclipse.org/r/102810
New Gerrit change created: https://git.eclipse.org/r/102809
New Gerrit change created: https://git.eclipse.org/r/102811
What about the performance with the throttler? Do you see any improvement? I think you can try to set the timer to 16ms (to reach 60FPS). In my experience, it does not impact the performance.
It has the desired effect, i.e. the dialogs improved performance. Other usages of Throttler also used 100ms and I think this value is OK. No one can recognize changing labels at 60 FPS.
Cool, let's wait for feedbacks about the new public API in JFace. Dani, Lars, any opinion?
Does changing a public method from synchronous to asynchronous enable a specific API policy?
(In reply to Mickael Istria from comment #11) > Does changing a public method from synchronous to asynchronous enable a > specific API policy? Just my 2 cents: Possible breaking behavior change. But it also depends on what the javadoc says and how severe is the behavior change for the client.
(In reply to Andrey Loskutov from comment #12) > (In reply to Mickael Istria from comment #11) > > Does changing a public method from synchronous to asynchronous enable a > > specific API policy? > Just my 2 cents: Possible breaking behavior change. But it also depends on > what the javadoc says and how severe is the behavior change for the client. Ok, let's ask for PMC opinion on this one.
Wouldn't it be simpler, from API POV, to have the updateLabel method (protected API) remain as it, and to introduce a "queueUpdateLabel" or similar method that would go through the Throttler and make all usage of the updateLabel() method in the classes replaced by this queueUpdateLabel() ? I think it would be the same behavior change but wouldn't affect API, so it should be OK to merge it in a minor version.
Good idea! However, we still need the Throttler move first.
(In reply to Mickael Istria from comment #14) > Wouldn't it be simpler, from API POV, to have the updateLabel method > (protected API) remain as it, and to introduce a "queueUpdateLabel" or > similar method that would go through the Throttler and make all usage of the > updateLabel() method in the classes replaced by this queueUpdateLabel() ? > I think it would be the same behavior change but wouldn't affect API, so it > should be OK to merge it in a minor version. See my comment in the Gerrit change.
(Fixing Dani pmc_reviewed flag as per discussion on Gerrit)
(In reply to Mickael Istria from comment #17) > (Fixing Dani pmc_reviewed flag as per discussion on Gerrit) Fine with me, but strange that you can do that ;-).
(In reply to Dani Megert from comment #18) > Fine with me, but strange that you can do that ;-). It worked but it did change the owner of the flag from you to me (which wasn't desired).
Gerrit change https://git.eclipse.org/r/102809 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=c85225ae9e9a28540966cac66891999094b1f769
New Gerrit change created: https://git.eclipse.org/r/106295
Gerrit change https://git.eclipse.org/r/106295 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=542b3cffe4f09d088f5c9ea5fe20522833094411
Gerrit change https://git.eclipse.org/r/102810 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=7408d6ae8f523df33b024b48f4a2f5275137d42f
Thank you Karsten! I'm actually not sure this has to get into the N&N as a dedicated entry. Maybe it's better to add a note about the various performance issue in the final N&N for Photon. What do you think?
I agree to collect performance news to one. Usually you have screenshots in N&N for separate entries, and this is nothing you can capture in a screenshot. We could leave the keywords and select them in a query later.
I suggest adding the new Throttler API to the N&N M3. Other projects might want to use it also to improve performance.
New Gerrit change created: https://git.eclipse.org/r/109963
Gerrit change https://git.eclipse.org/r/109963 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=743ce3c8e85481e447729e0c029e6b8ea9887fda
