Batch window changes events on XCB
ClosedPublic

Authored by davidedmundson on Aug 23 2017, 11:04 AM.

Details

Summary

In a log from someone talking about high CPU we can see get multiple X
events for the same window as multiple events, but directly next to each
other. This causes the TaskManager to process changes multiple times
instead of just once which is a waste.

An example is just pressing "enter" in konsole, which will pointlessly
update the title.

This causes problems for expensive app lookup and also QML performs text layouts immediately so any text changes cause quite large CPU usage if done more than 60fps; especially a task text resizing
could result in resizing the entire panel.

Something not relevant in kwin that also monitors these rolls.

This class sits between KWindowSystem and XWindowTasksModel
transparently buffering the changes.

CCBUG: 378010
BUG: 365317

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a project: Plasma. · View Herald TranscriptAug 23 2017, 11:04 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

In general I'm against this as we would also need the same in KWin and every other user. Theproblem here is Firefox: that one should be fixed.

hein added a subscriber: hein.Aug 23 2017, 11:48 AM

I'm in general for this. It's listed in the TODO file as one of the remaining open tasks for the lib, as the old lib did this but it got dropped along the way.

However, I would like it to be implemented as a proxy pass (e.g. in WindowTasksModel) so it can be shared between X11 and Wayland instead of being implemented redundantly or encumbering that code, and so it's easy to disable when debugging something and having to reason about the code.

We don't want it for Wayland.

Surfaces are double buffered so all synced. We don't fully do that on the XDG side, but most of that is static anyway

Theproblem here is Firefox: that one should be fixed.

I can also cause trouble with VLC if I have a repeating playlist with a broken file in it, it will continuously cycle between "$title" and "VLC" and cause Plasma to freeze. We can't fix all apps…

hein added a comment.Aug 23 2017, 12:29 PM

We don't want it for Wayland.

Surfaces are double buffered so all synced. We don't fully do that on the XDG side, but most of that is static anyway

Can you explain that more? The reason the old lib did this and the reason I'm aware of for wanting this is to prevent the TM being ddos'd by a rogue window updating its title thousands of times per second. How are we protected from this on Wayland currently?

Theproblem here is Firefox: that one should be fixed.

I can also cause trouble with VLC if I have a repeating playlist with a broken file in it, it will continuously cycle between "$title" and "VLC" and cause Plasma to freeze. We can't fix all apps…

As the window caption is ultimately set by KWin we can limit the update rate.

The reason the old lib did this and the reason I'm aware of for wanting this is to prevent the TM being ddos'd by a rogue window updating its title thousands of times per second. How are we protected from this on Wayland currently?

By ensuring KWin does a sane job. If a window can that way ddos Plasma it could also ddos KWin, which would be worse. But as KWin is the ultimate source to send the caption to KWin, we can limit this to e.g. once per second.

@hein, @davidedmundson if the main concern is too often update of title I suggest we implement limiting of updates in KWin. So the batch would happen in KWin, taken away stress from KWin and indirectly also from Plasma.

Against updating icons it won't help, though as KWin doesn't set this.

libtaskmanager/xwindowsystemeventbatcher.cpp
49

if we can depend on Qt 5.9 you could simplify the BATCH_TIME to either:

startTimer(std::chrono::milliseconds(10));

or together with C++14 to:

using namespace std::chrono_literals;
startTimer(10ms);
hein added a comment.Aug 23 2017, 9:18 PM

I'm not sure we only care about the title.

First of, icons: libtm usually ignores NET::WMIcon changes. It only looks at them for windows which hit the fallback path of using the WM icon. Of course a malicious window might be particularly likely to fall into that category, though.

Now, the roles that are generally interesting for ddos purposes are all of the roles that contribute to app identification and cause a cache eviction and re-running of the app matching heuristic. The heuristic is computationally expensive, and changes in the resulting app identity cause more computation like re-evaluation of grouping decisions.

Here's all the roles that cause this:

NET::WMName, NET::WMVisibleName, NET::WMPid, NET::WM2DesktopFileName, NET::WM2WindowClass

Some of these are non-issues (PID) and some are title. Leaves NET::WM2DesktopFileName & NET::WM2WindowClass.

Window class is not allowed to change by ICCCM. For desktop file we could decide that we don't monitor changes as it is our own property.

hein added a comment.Aug 24 2017, 8:48 AM

Sounds good, then we really only care about the title and icon.

Let's do the title in KWin, then, AIUI? But we still need this patch for the icon. But we could now limit it to windows that are actually on the fallback path that does anything in response to icon changes. d_ed, could you revise to make this more targeted?

In D7481#139276, @hein wrote:

Sounds good, then we really only care about the title and icon.

Let's do the title in KWin, then, AIUI? But we still need this patch for the icon. But we could now limit it to windows that are actually on the fallback path that does anything in response to icon changes. d_ed, could you revise to make this more targeted?

I just checked KWin source and in the X11 case it might not work as I thought. KWin does not always set the visible name. We could change KWin in that regard, but it only makes sense if libtm ignores the "other" window caption properties. If we do that we might end up breaking libtm in combination with other WMs.

hein added a comment.Aug 24 2017, 3:43 PM

I had a feeling it was like that, WM::Name always coming from KWin seemed weird somehow.

Ok, so we need the name props and selectively icon.

cfeck added a subscriber: cfeck.Oct 5 2017, 12:37 AM

Is there a consensus if this is still needed?

ngraham added a subscriber: ngraham.Oct 5 2017, 4:23 AM
hein added a comment.Oct 5 2017, 12:44 PM

Yes it is, pending rework.

hein added a comment.Oct 24 2017, 7:11 AM

David, ping?

@davidedmundson, what are our next steps here?

ngraham edited the summary of this revision. (Show Details)Dec 13 2017, 8:43 PM

Pingeliping.

libtaskmanager/xwindowsystemeventbatcher.cpp
35

Avoid double lookup

36

Why emit a window change just before you emit a removal? Or is that what KWindowSystem usually does and we rely on that?

46

Look up only once by caching reference?

58

Coding style

Monthly ping :)

davidedmundson added inline comments.Mar 28 2018, 3:53 PM
libtaskmanager/xwindowsystemeventbatcher.cpp
36

In a version where we cache all properties, I didn't want to have anything important missed.

If we only cache the name, then you're right, I don't need to.

Changed to a version that only caches names as requested.

davidedmundson edited the summary of this revision. (Show Details)Mar 28 2018, 4:41 PM
davidedmundson edited the test plan for this revision. (Show Details)
davidedmundson retitled this revision from RFC: Batch window changes events on XCB to Batch window changes events on XCB.Mar 28 2018, 4:46 PM
davidedmundson edited the summary of this revision. (Show Details)

debug--

hein accepted this revision.Mar 29 2018, 4:59 AM
This revision is now accepted and ready to land.Mar 29 2018, 4:59 AM
hein added a comment.Mar 29 2018, 5:33 AM

Small request: The TODO file in the lib's folder has a note about this. You can remove it now that it's done.

broulik accepted this revision.Mar 29 2018, 7:10 AM
broulik added inline comments.Mar 29 2018, 8:29 AM
libtaskmanager/xwindowsystemeventbatcher.cpp
70

Please check event->timerId() against m_timerId, plasmashell keeps hogging 100% CPU when I have certain windows focused (e.g. ksysguard) as this method is entered a ton of times with timer ids other than our own.

broulik added inline comments.Mar 29 2018, 10:54 AM
libtaskmanager/xwindowsystemeventbatcher.cpp
70

Even with that plasmashell constantly consumes a ton of CPU and becomes sluggish depending on what window is currently focused and what you're doing :/ Perhaps QBasicTimer is a better choice?

proper timer usage

broulik accepted this revision.Mar 29 2018, 2:21 PM
This revision was automatically updated to reflect the committed changes.