Community
Participate
Working Groups
I recently had a UI freeze in my log. After investigation, I discovered that SashLayout is forcing a synchronous uncached layout and full repaint on each mouse event. Neither of these are necessary. They can be replaced by a call to requestLayout. It performs an asynchronous layout that merges with other layout events, doesn't block the event loop, and will only repaint those regions of the screen whose size actually changed.
New Gerrit change created: https://git.eclipse.org/r/90460
Gerrit change https://git.eclipse.org/r/90460 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=43d5cf6ced0e069f1a049bfb5fe16026a0ab3edd
SashLayout is not API, so we can simply remove "layoutUpdateInProgress". Gerrit patch to follow.
New Gerrit change created: https://git.eclipse.org/r/90493
Why is it not API? It looks to me like an exported package without the word internal. Doesn't that make it API?
(In reply to Stefan Xenos from comment #5) > Why is it not API? It looks to me like an exported package without the word > internal. Doesn't that make it API? Everything which is marked as x-internal or x-friends in the MANIFEST.MF is not API. Export-Package: org.eclipse.e4.ui.internal.workbench.renderers.swt;x-friends:="org.eclipse.ui.workbench", org.eclipse.e4.ui.workbench.renderers.swt;x-friends:="org.eclipse.e4.ui.workbench.addons.swt,org.eclipse.ui.workbench"
Gerrit change https://git.eclipse.org/r/90493 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=e0219275e7374a09c884b4f2975059f6b29d61f2
This change ruines drag/drop views re-layout. Just grab the resize handler shown if you mouse hovers in the top left corner of the problems view and try to resize vies. The resize doesn't follow the mouse anymore, it feels as if this waits seconds to relayout itself.
New Gerrit change created: https://git.eclipse.org/r/90889
Gerrit change https://git.eclipse.org/r/90889 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=11537c7a07329f8e242e6f4dfedf4cb6b736a8a1
Andrey, what platform are you using? It works great for me on GTK.
(In reply to Stefan Xenos from comment #11) > Andrey, what platform are you using? It works great for me on GTK. Windows 10, sorry.
Either we'll need a plaform-specific fix or we'll need to fix the windows port of SWT such that it repaints properly in such situations. My inclination is the latter, since we've had other reports of windows not repainting and there's quite a few workarounds in place. Fixing this at the SWT layer would eliminate the need for such platform-specific workarounds and would also fix other similar bugs.
Mass move. Please move back to M6, if necessary
I assume this is to late for 4.7. Stefan, please move back in case you have something planned here.
Nothing planned for M7.
Removing target milestone for all bugs that are not major or above.
> Removing target milestone for all bugs that are not major or above. Of course I meant "major or below". Sorry for the noise!
*** Bug 501227 has been marked as a duplicate of this bug. ***
Woh, a full synchronous layout call within a mouse move listener in SashLayout? No wonder the UI is laggy.
New Gerrit change created: https://git.eclipse.org/r/154452
See also Bug 558290.
Created attachment 281231 [details] Effect of the latest change (defered layout) on Windows 7 (In reply to Stefan Xenos from comment #0) > I recently had a UI freeze in my log. After investigation, I discovered that > SashLayout is forcing a synchronous uncached layout and full repaint on each > mouse event. Neither of these are necessary. They can be replaced by a call > to requestLayout. > > It performs an asynchronous layout that merges with other layout events, > doesn't block the event loop, and will only repaint those regions of the > screen whose size actually changed. I don't think a synchronous layout/repaint is avoidable. While dragging the sash you want to see the changes. I would expect any asynchronicity or deferral to be visible as lagging or tearing. (what Andrey described in comment 8) In fact I'm surprised it works on Linux. I assume it does the layout quite fast after request and only reduce the total number of layouts a bit. Imo 'blocking' the event loop (I would not expect it is fully blocked) isn't so bad in this context. I would expect user produced messages are most impotent with regard to responsiveness but user is already engaged in changing the layout. Where I agree is the uncached and probably the full repaint part. Unfortunately simply changing the layout call to not flush caches will not help because the layout method of SashLayout is ignoring this flag atm. So (from my current state of knowledge) improving the layout implementation and reducing the area of redraw is something that should be possible and improve the freezing part. Such a change isn't be trivial but I would give it a try. PS: the tearing seen in attached video was almost nothing influenced by the capturing. It might cleanup the tearing a bit more often without but the overall effect is absolutely the same.
(In reply to Paul Pazderski from comment #23) > So (from my current state of knowledge) improving the layout implementation > and reducing the area of redraw is something that should be possible and > improve the freezing part. Such a change isn't be trivial but I would give > it a try. Thanks. I read this as that you are planning to give it try.
(In reply to Lars Vogel from comment #24) > (In reply to Paul Pazderski from comment #23) > > > So (from my current state of knowledge) improving the layout implementation > > and reducing the area of redraw is something that should be possible and > > improve the freezing part. Such a change isn't be trivial but I would give > > it a try. > > Thanks. I read this as that you are planning to give it try. Yes, as long as nobody comes and shows me where my explanation is totally wrong.
> While dragging the sash you want to see the changes. I would expect > any asynchronicity or deferral to be visible as lagging or tearing. "Deferring" a layout only defers it until control returns to the event loop, which is generally a matter of milliseconds. You don't see it. Normally it still happens before the next vsync, so it's generally completely invisible. However, this deferral is extremely important for the asymptotic runtime of the layouts, since multiple controls can (and do) change on a single pass through the event loop. If each control in your hierarchy performed a synchronous layout the runtime would be O(n^2). If each control performs an asynchronous layout, the runtime is O(n). It seems like each time I filed one of these, someone encounters asynchronous layouts for the first time and says they shouldn't be asynchronous. I then generally responded by posting a benchmark comparing that particular layout coded in synchronous versus asynchronous algorithms that shows the asynchronous version outperforming the synchronous one by at least an order of magnitude. Since I don't work on Eclipse anymore, I won't code up another one this time. However, if you search through bugzilla you'll find at least 4 other instances of this discussion with sample code and benchmarks. (The first occurrence was with Steve Northover. It should be easy to find since the benchmarks there were the reason he introduced deferred layouts to SWT in the first place.) Rendering is (and needs to be) an asynchronous pipeline. App code modifies controls, which dirty layouts. Then a layout pass happens asynchronously which dirties controls for painting. Trying to force a synchronous layout or synchronous paint never makes things faster.
(In reply to Stefan Xenos from comment #26) > Rendering is (and needs to be) an asynchronous pipeline. App code modifies > controls, which dirty layouts. Then a layout pass happens asynchronously > which dirties controls for painting. Trying to force a synchronous layout or > synchronous paint never makes things faster. Thanks Stefan for still listening it. The issue here seems to be that Windows SWT does not repaint if no sync layout call is triggered. Maybe this is buggy on SWT Win. Works fine on Linux, not sure about Mac.
> The issue here seems to be that Windows SWT does not repaint if no sync layout > call is triggered. Fair enough. But the fix would be to figure out why the async layouts aren't running, not force a synchronous one.
(In reply to Stefan Xenos from comment #26) > > While dragging the sash you want to see the changes. I would expect > > any asynchronicity or deferral to be visible as lagging or tearing. > > "Deferring" a layout only defers it until control returns to the event loop, > which is generally a matter of milliseconds. You don't see it. Normally it > still happens before the next vsync, so it's generally completely invisible. Right. I overlooked that deferred layout is run after *any* OS event. It kind of crossed my mind it would layout after current event queue was exhausted. I'm a bit biased because my first deeper contact with SWT layouts was a bug where deferred layout failed and was deferred for seconds, minutes or forever. (In reply to Stefan Xenos from comment #28) > > The issue here seems to be that Windows SWT does not repaint if no sync layout > > call is triggered. > > Fair enough. But the fix would be to figure out why the async layouts aren't > running, not force a synchronous one. One of the reasons is that another patch for deferred layout problems was applied for GTK but not for Windows and Mac. https://bugs.eclipse.org/bugs/show_bug.cgi?id=416845 But even with this change applied there remain ugly rendering problems on Windows.
Matthias checked MacOS and it also behaves fine. I plan to push later tonight a patch which uses requestLayout on Linux and MacOS and the evil layout call only on Windows. No need to suffer from Windows bugs on other OSs. From Paul: > One of the reasons is that another patch for deferred layout problems was >applied for GTK but not for Windows and Mac. > https://bugs.eclipse.org/bugs/show_bug.cgi?id=416845 Thanks Paul for looking into this. I don't see a patch in Bug 416845. Could you point me to the corresponding commit / bug report? Or even better, could you open a bug for Windows / Mac for the missing fix?
(In reply to Lars Vogel from comment #30) > Matthias checked MacOS and it also behaves fine. With or without bug 416845? > From Paul: > > > One of the reasons is that another patch for deferred layout problems was >applied for GTK but not for Windows and Mac. > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=416845 > > Thanks Paul for looking into this. I don't see a patch in Bug 416845. Could > you point me to the corresponding commit / bug report? Or even better, could > you open a bug for Windows / Mac for the missing fix? One reason you see no patch is because I linked the wrong bug in my comment. Correct is bug 354762. Added the corresponding commit there. I'll not open for Mac. It seem to work fine on Mac and if it also works with deferred layout I would not add the update() without reason. For Windows the change does not fix the problem. I opened bug 558392 instead and depending on the outcome it may include this change. Regardless of the sync vs async stuff I still think the SashLayout layout() implementation is improvable and should not ignore the flushCache flag.
(In reply to Paul Pazderski from comment #31) > (In reply to Lars Vogel from comment #30) > > Matthias checked MacOS and it also behaves fine. > > With or without bug 416845? Sorry. Copied wrong number again. With or without bug 354762?
(In reply to Paul Pazderski from comment #31) > Regardless of the sync vs async stuff I still think the SashLayout layout() > implementation is improvable and should not ignore the flushCache flag. +1 I opened Bug 558394 for that.
(In reply to Paul Pazderski from comment #32) > (In reply to Paul Pazderski from comment #31) > > (In reply to Lars Vogel from comment #30) > > > Matthias checked MacOS and it also behaves fine. > > > > With or without bug 416845? > > Sorry. Copied wrong number again. With or without bug 354762? I just fetched https://git.eclipse.org/r/#/c/153948/ into my workspace. That's all.
Created attachment 281257 [details] Deferred Layout on Linux
Gerrit change https://git.eclipse.org/r/153948 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=7163ea3ccb1477b171f47d3529bcfcdd2fc0a3ba
Marking the follow-up bugs as dependent bugs so that he can easier correlate them. Marking as fixed as Mac and Linux benefit from this change. Likelihood that this causes also issues on these platform is there, so we may see a reopen soon. :-)
