[libkwineffects] Replace property name lookup with calling the virtual methods
ClosedPublic

Authored by davidedmundson on Nov 1 2018, 11:44 PM.

Details

Summary

EffectWindow proxies its properties from the client/deleted's properties.

QObject::property(char*) is a slow string search. It's a loop
not a hash lookup!

QML is different, there's a property cache.

It's fetched multiple times for every window in every paint of some
effects (such as blur).

Hotspot showed it as 5% of the standard render pass (X11) with nothing
in kwin animating.

This patch replaces the macro that does parent()->property("propertyName")
with a macro calling the relevant function directly without metaobjects.

All custom proprties on deleted have to be moved to virtual's on the toplevel.

Test Plan

Existing unit tests
Ran it for a bit

Diff Detail

Repository
R108 KWin
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5522
Build 5540: arc lint + arc unit
davidedmundson created this revision.Nov 1 2018, 11:44 PM
Restricted Application added a project: KWin. · View Herald TranscriptNov 1 2018, 11:44 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson requested review of this revision.Nov 1 2018, 11:44 PM
davidedmundson added inline comments.Nov 2 2018, 1:44 AM
effects.h
508

Ignore these changes. Will fix in the morning.

broulik added a subscriber: broulik.Nov 2 2018, 3:35 AM

+1 noticed that too in hotspot but didn't think of that solution.

zzag added a subscriber: zzag.Nov 2 2018, 9:49 AM
autotests/test_window_paint_data.cpp
179

Missing override.

effects.cpp
1758

We probably don't need this comment.

1798

Missing whitespace between the type and "*".

effects.h
360

Please remove "virtual".

439

Missing whitespace between the type and "*".

toplevel.h
464 ↗(On Diff #44680)

It's off by one space.

467–471 ↗(On Diff #44680)

Can you please add comments for these too?
For example, that's not really clear what "keepAbove" means. One could think that's an action.

472–476 ↗(On Diff #44680)

Please re-format it.

Oh, I didn't think to look.

It is possible to do something with just one string lookup of the object type and then keep a hash if we desperately want to keep it.

Though we've failed to keep ABI for practically every release in the 5.x series even with this. I think /if/ ABI compatibility is a worthwhile goal we should be using pimpl and having EffectWindowImpl subclass EffectWindow::Private which contains the virtuals - but that would be a task for another day.

zzag added a comment.Nov 2 2018, 1:01 PM

Though we've failed to keep ABI for practically every release in the 5.x series even with this. I think /if/ ABI compatibility is a worthwhile goal we should be using pimpl and having EffectWindowImpl subclass EffectWindow::Private which contains the virtuals - but that would be a task for another day.

Yeah, it would be great to do that way.

Some notes: when we introduced the property solution we assumed it's not more expensive than the virtual lookup. When we introduced it we were still adding lots of new methods to the API. Furthermore we did not increase the so version and instead used internal api versioning. Distros were complaining a lot about the situation. At that time we expected that 3rd party binary effects would be added.

Today we do want 3rd party to use scripting, we do so versioning and effectwindow doesn't change that often. So in summary: go for it.

graesslin requested changes to this revision.Nov 2 2018, 7:20 PM

Please do not add the methods to Toplevel. This would break the semantics of this class hierarchy. The idea of Toplevel is being the base of a managed and an unmanaged window. Having methods in Toplevel which don't fit the semantics of an unmanaged window is problematic. An unmanaged cannot be keepBeliw and it's caption cannot change as it doesn't have one. I would prefer if the API stays clean to express these differences (that's btw. the reason for introducing AbstractClient).

The code before the introduction of the properties used dynamic casts to figure out whether it's an unmanaged or client and called the appropriate methods. I'm sure there's a neat trick allowing to keep the semantics and not needing additional performance.

This revision now requires changes to proceed.Nov 2 2018, 7:20 PM
davidedmundson planned changes to this revision.Nov 4 2018, 9:59 PM

when we introduced the property solution we assumed it's not more expensive than the virtual lookup.

Sure, I wasn't trying to criticise the original code (except maybe Qt's code)
Before blur/background constrast we didn't have effects checking window properties every frame so it wouldn't have been a big issue.

Please do not add the methods to Toplevel.

Ok

. I'm sure there's a neat trick

We have two main options:

  • We can introduce a class inheriting toplevel that both abstract client and deleted then inherit from
  • We make EffectWindowImpl know about deleted in the newer WINDOW_HELPER_DEFAULT

I'm leaning towards the second option

when we introduced the property solution we assumed it's not more expensive than the virtual lookup.

Sure, I wasn't trying to criticise the original code (except maybe Qt's code)

I didn't read it as criticism, just wanted to give some background information.

Please do not add the methods to Toplevel.

Ok

. I'm sure there's a neat trick

We have two main options:

  • We can introduce a class inheriting toplevel that both abstract client and deleted then inherit from
  • We make EffectWindowImpl know about deleted in the newer WINDOW_HELPER_DEFAULT

    I'm leaning towards the second option

Yeah I would also lean towards second. Maybe effectwindowimpl could be templated?

No changes to toplevel, EffectWindowImpl now does the relevant switching.

zzag added a comment.Nov 29 2018, 11:30 PM

Maybe that's a problem on my side, but... testWindowPaintData doesn't pass.

zzag added inline comments.Nov 29 2018, 11:32 PM
effects.cpp
1751
#undef TOPLEVEL_HELPER
1775

Please delete this comment when you're about to push this change.

  • Remove the pointless comment that had got copy pasted into new code
  • undef macros after use
  • Also move the cached properties from EffectWindow to EffectWindowImpl
davidedmundson marked 4 inline comments as done.

trivial whitespace

zzag accepted this revision.Nov 30 2018, 10:40 AM

Looks good to me.

Hotspot showed it as 5% of the standard render pass (X11) with nothing in kwin animating.

I have a feeling that this statement might be misinterpreted by some folks, like "KWin is now faster". Which is true, but if you use a standalone X11 session, you probably won't notice that.

effects.cpp
1672

Because we're inside of KWin, we can just use qobject_cast. :-)

This revision was not accepted when it landed; it landed in state Needs Review.Nov 30 2018, 11:23 AM
This revision was automatically updated to reflect the committed changes.