[effects/presentwindows] Avoid potential freeze during fill-gaps
ClosedPublic

Authored by ekurzinger on Oct 17 2018, 4:29 PM.

Details

Summary

When using the natural layout algorithm with the fill-gaps option, a small
error (less than one) is introduced in windows' aspect ratio each time they are
enlarged due to floating-point roundoff.

Currently, the algorithm computes the width and height enlargement factors and
then attempts to enlarge in each of the four possible directions, repeating
until it can't enlarge any windows any further. Hence, this aspect ratio error
can be multiplied by up to four. Especially for small, long, and narrow
windows, this can result in a total error of greater than one by the end of
that loop iteration. If this occurs, on subsequent iterations the height
enlargement factor might then be computed as negative violating some of the
core assumptions of the algorithm and resulting in the loop iterating endlessly
until one of the window dimensions overflows, freezing the program for up to
several minutes.

To fix this, the height enlargement factor should be re-computed based on the
new width each time the window is enlarged, ensuring the error introduced in
the aspect ratio never exceeds one.

BUG: 364709
BUG: 380865
BUG: 368811

FIXED-IN: 5.15.0

Test Plan

The most reliable way to reproduce the freeze seems to be to activate the desktop-grid effect while a tool-tip window is fading in.
Ensure desktop-grid is configured to use present windows, and that present windows is configured to use the natural layout algorithm with the fill gaps option selected.

The freeze is still intermittent, but using this method should be able to be triggered within about 10 tries without this fix.
After applying the fix, the freeze has never been observed.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ekurzinger requested review of this revision.Oct 17 2018, 4:29 PM
ekurzinger created this revision.
ekurzinger edited the summary of this revision. (Show Details)
ekurzinger edited the summary of this revision. (Show Details)
zzag retitled this revision from [effects/presentwindows] avoid potential freeze during fill-gaps to [effects/presentwindows] Avoid potential freeze during fill-gaps.Oct 17 2018, 4:41 PM
zzag edited the summary of this revision. (Show Details)
zzag removed a reviewer: kwin.EditedOct 17 2018, 4:58 PM
zzag added a subscriber: zzag.

Hi, thank you for your patch! :-)

I didn't look at the code, but recomputing height after each enlargement is sensible.

The only question: did you find a way to reproduce the freezing? If so, please describe how to get KWin frozen in the test plan section. (I tried many times to get KWin frozen, but unfortunately couldn't succeed)

Restricted Application added a subscriber: kwin. · View Herald TranscriptOct 17 2018, 4:58 PM
ekurzinger added a comment.EditedOct 17 2018, 5:23 PM

Hi Vlad,

The most reliable way to reproduce the freeze in my experience was to trigger the desktop-grid effect while a tool-tip was fading in. Actually, it was one of our users that noticed this. They uploaded a video here demonstrating https://youtu.be/_l1ab_PLP7Y

Doing this, I found I could generally get the freeze within about 5 - 10 tries (without this fix). It might also help to lower the animation speed under compositor settings to make the timing more forgiving. The reproducibility does seem to be affected by seemingly random factors though. You might need to play around with the exact position / size of any windows or which tool-tip you use.

As I mentioned in the summary, I suspect this is because tool-tip windows are fairly small as well as long and narrow so the aspect ratio error ends up being relatively high when presentwindows tries to enlarge them.

P.S. Thanks for fixing up the revision title / version field, this is my first KDE patch :)

ekurzinger edited the test plan for this revision. (Show Details)Oct 17 2018, 5:36 PM
zzag added a comment.Oct 17 2018, 5:52 PM

I spent last 20 minutes toggling the Present Windows and the Desktop Grid effect without any success. Can you please provide more information how to reproduce the bug? How many desktops you have? How many windows? Are they maximized? etc.

Sure, I have a single dolphin window positioned in the top-left corner of the leftmost desktop with dimensions 750 by 500. For some reason, the tool-tip for the "Split" button seems to be the most consistent way, but maybe that's just my imagination.
I have a single 1920x1200 monitor, and 3 virtual desktops in a single row. I've attached my kwinrc file in case there's anything else relevant there. Animation speed is set to 4, and the compositor backend is set to OpenGL 3.1

zzag added inline comments.Oct 18 2018, 12:58 PM
effects/presentwindows/presentwindows.cpp
1472–1473

This one is redundant.

zzag requested changes to this revision.Oct 18 2018, 2:12 PM

The freeze is still intermittent, but using this method should be able to be triggered within about 10 tries without this fix.

Well, I wasted hours trying to reproduce the bug with no luck.

Please add a test that reproduces the freeze and this patch fixes it.

This revision now requires changes to proceed.Oct 18 2018, 2:12 PM
In D16278#345365, @zzag wrote:

The freeze is still intermittent, but using this method should be able to be triggered within about 10 tries without this fix.

Well, I wasted hours trying to reproduce the bug with no luck.

So did I! Good job.

Please add a test that reproduces the freeze and this patch fixes it.

That might be a little bit much to request given that we don't have tests for such complex effects yet.

zzag accepted this revision.Oct 20 2018, 9:36 AM

That might be a little bit much to request given that we don't have tests for such complex effects yet.

Yeah, maybe...

Recomputing heightDiff after each enlargement is sensible, so I think this is good to go.

@ekurzinger Do you have commit access? If not, can you please provide your real name and email for commit authorship information?

This revision is now accepted and ready to land.Oct 20 2018, 9:36 AM
ekurzinger updated this revision to Diff 43972.Oct 20 2018, 3:02 PM
ekurzinger removed a reviewer: romangg.
In D16278#345365, @zzag wrote:

The freeze is still intermittent, but using this method should be able to be triggered within about 10 tries without this fix.

Well, I wasted hours trying to reproduce the bug with no luck.

So did I! Good job.

Yeah, sorry about that, guys. It took me quite a while to get a repro setup as well. I guess it's just really, really finicky.
There seem to be a fair number of users on the bug tracker experiencing the issue, though. Perhaps it will be worth checking with them after this is merged to verify that it does indeed fix things?

In D16278#346210, @zzag wrote:

Recomputing heightDiff after each enlargement is sensible, so I think this is good to go.

@ekurzinger Do you have commit access? If not, can you please provide your real name and email for commit authorship information?

I do not. My full name is Erik Kurzinger and my email is ekurzinger@nvidia.com
Also, I've removed that last superfluous re-computation - thanks for catching that, Vlad!

This revision was automatically updated to reflect the committed changes.