Merge lp://staging/~compiz-team/compiz/compiz.performance_1027211.2 into lp://staging/compiz/0.9.9

PASSED: Continuous integration, rev:3536
http://jenkins.qa.ubuntu.com/job/compiz-ci/312/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/312/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/312//rebuild/?

Confirmed moving windows is much improved. There is no improvement with resizing though. I still encounter exactly the same freezes as mentioned previously:
  1. XSync() inside PrivateWindow::updateRegion()
  2. XShapeGetRectangles()
So please either:
  (a) Don't touch XShape* in this proposal, and leave resize fixes for a later proposal; or
  (b) Fix XShape* properly.
Given the large size of this proposal already, and that a proper fix for XShape should be to simply call it much less often (i.e. only for shaped windows), I recommend (a).

Secondly, I do support the concept of disabling lazy positioning in theory but would have to get my test machine (Atom) for that upgraded and working again before I approve it.

Also, without too much verbosity, why is it we need to implement window movement completely synchronously? My understanding is that waiting and locking should never be required when the user moves a window.

A window move should simply involve two events:
  1. Motion event -> move plugin -> send request to X server (XConfigureWindow I think).
and some time later (should not care when):
  2. XConfigureNotify (or whatever) -> core plugin -> compiz window position updated.

If this is lazy positioning then maybe I do support it. But historically we seem to have a lot more code than should be necessary to implement movement so I'm not sure.

I mean; my understanding of lazy positioning is that we update the compiz position without waiting for the server to tell us it's done. Therefore the above method counts as non-lazy movement, but also does not require any waiting or locking. I think this is contrary to the proposed and established non-lazy algorithm that does sync for some reason.

If we want to keep lazy mode as an option and it does bypass the server as I suspect then OK. However I think at least non-lazy mode should be implemented as above, so that either way you never need to lock or sync to move a window.

On Mon, Dec 10, 2012 at 2:16 PM, Daniel van Vugt
<email address hidden> wrote:
> Review: Needs Fixing
>
> Confirmed moving windows is much improved. There is no improvement with resizing though. I still encounter exactly the same freezes as mentioned previously:
> 1. XSync() inside PrivateWindow::updateRegion()
> 2. XShapeGetRectangles()
> So please either:
> (a) Don't touch XShape* in this proposal, and leave resize fixes for a later proposal; or
> (b) Fix XShape* properly.
> Given the large size of this proposal already, and that a proper fix for XShape should be to simply call it much less often (i.e. only for shaped windows), I recommend (a).

Ah, we actually need to touch it otherwise this proposal won't work
without it, though its not a very big touch, and more or less just a
wrapper around it to make sure that when it is called, we release the
geometry queue for that window.

The reason for that being that XShapeGetRectangles is dependent on us
having the most recent server size of the window actually posted to
the server by that point. If it isn't, then you'll get the wrong
client shape, and that will be incorrectly applied to the frame!

>
> Secondly, I do support the concept of disabling lazy positioning in theory but would have to get my test machine (Atom) for that upgraded and working again before I approve it.
>
> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.performance_1027211.2/+merge/138800
> Your team Compiz Maintainers is subscribed to branch lp:compiz.

--
Sam Spilsbury

On Mon, Dec 10, 2012 at 2:41 PM, Daniel van Vugt
<email address hidden> wrote:
> Also, without too much verbosity, why is it we need to implement window movement completely synchronously? My understanding is that waiting and locking should never be required when the user moves a window.
>
> A window move should simply involve two events:
> 1. Motion event -> move plugin -> send request to X server (XConfigureWindow I think).
> and some time later (should not care when):
> 2. XConfigureNotify (or whatever) -> core plugin -> compiz window position updated.
>
> I mean; my understanding of lazy positioning is that we update the compiz position without waiting for the server to tell us it's done. Therefore the above method counts as non-lazy movement, but also does not require any waiting or locking. I think this is contrary to the proposed and established non-lazy algorithm that does sync for some reason.
>
> If we want to keep lazy mode as an option and it does bypass the server as I suspect then OK. However I think at least non-lazy mode should be implemented as above, so that either way you never need to lock or sync to move a window.

Ah, I think there's a mixup of terminology here.

We're still completely async, both this branch and before this branch.
ConfigureWindow requests are in themselves async, you can post as many
as you want. There's absolutely no waiting for the server.

The problem is that the nvidia driver can't handle it. So instead of
sending it lots and lots of ConfigureWindow requests, we let plugins
"lock out" core from doing that until it makes sense for us to do it.
This means that:

1. In the case of "lazy positioning" being disabled, we just wait
until just before the next frame and post the request
2. In the case of "lazy positioning" enabled, we wait until the window
is ungrabbed.

Both are overridden in cases where we have to ask the server
something. But that's not too often.

> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.performance_1027211.2/+merge/138800
> Your team Compiz Maintainers is subscribed to branch lp:compiz.

--
Sam Spilsbury

It will take me a while to get the code compiled 32-bit and running on the Atom where I tested all the previous lazy-mode regressions. Until then, some style issues:

1. This is hard to read. I recommend (but don't require) all on one line...
48 +namespace compiz
49 +{
50 +namespace window
51 +{
52 +namespace configure_buffers
53 +{

2. MOVE_WINDOW (w) - Using macros to declare local variables via side-effect is dangerous and very hard to understand if you're new to compiz. I suggest not doing this in future code. Instead use an explicit:
    MoveWindow *mw = MoveWindow::get(w);

3. From a high-level perspective, even if the implementation turns out to be right then I think maybe more analysis was required. This fix is an example of where the medicine could be worse than the disease. It's only useful to nvidia because nvidia is buggy IMHO. And the price to pay is more abstraction and complication that hinders maintainability of the code. We might be better off not trying to "fix" window movement any further. Though resizing needs work still.

OK, verified window movement is absolutely perfect on the Atom with this change. So no non-lazy-mode regression to worry about.

Just need (want) to reconsider the style issues and the philosophical point above.

FYI, window resizing on the Atom (i915) freezes just like with Nvidia so that's another good test platform.

> It will take me a while to get the code compiled 32-bit and running on the
> Atom where I tested all the previous lazy-mode regressions. Until then, some
> style issues:
>
> 1. This is hard to read. I recommend (but don't require) all on one line...
> 48 +namespace compiz
> 49 +{
> 50 +namespace window
> 51 +{
> 52 +namespace configure_buffers
> 53 +{
>
> 2. MOVE_WINDOW (w) - Using macros to declare local variables via side-effect
> is dangerous and very hard to understand if you're new to compiz. I suggest
> not doing this in future code. Instead use an explicit:
> MoveWindow *mw = MoveWindow::get(w);

+1

>
> 3. From a high-level perspective, even if the implementation turns out to be
> right then I think maybe more analysis was required. This fix is an example of
> where the medicine could be worse than the disease. It's only useful to nvidia
> because nvidia is buggy IMHO. And the price to pay is more abstraction and
> complication that hinders maintainability of the code. We might be better off
> not trying to "fix" window movement any further. Though resizing needs work
> still.

It is a little more abstract and complex, yes, however having an unusable system on nvidia is not an acceptable alternative either.

I don't think the code is "unmaintainable", I think it just works in a way that is a tad more complex and different to the way that you would expect. Its fully tested and encapsulated from the other code, so future maintainers don't need to worry about inadvertently breaking it (except for their usage of the client hooks).

In addition, if there are any problems you can just "turn it off" - just don't take a lock on the configure request queue in the opengl plugin.

Speaking of medicine worse than the disease, you just named one of my favourite pieces of music :P (http://www.youtube.com/watch?v=htGmTJNsOr0)

>
> +1
>
> >
> > 3. From a high-level perspective, even if the implementation turns out to be
> > right then I think maybe more analysis was required. This fix is an example
> of
> > where the medicine could be worse than the disease. It's only useful to
> nvidia
> > because nvidia is buggy IMHO. And the price to pay is more abstraction and
> > complication that hinders maintainability of the code. We might be better
> off
> > not trying to "fix" window movement any further. Though resizing needs work
> > still.
>
> It is a little more abstract and complex, yes, however having an unusable
> system on nvidia is not an acceptable alternative either.
>
> I don't think the code is "unmaintainable", I think it just works in a way
> that is a tad more complex and different to the way that you would expect. Its
> fully tested and encapsulated from the other code, so future maintainers don't
> need to worry about inadvertently breaking it (except for their usage of the
> client hooks).
>

On the same note.

Over my many years of developing compiz, I've often found that the little quirks between drivers, opengl, the way the xserver works, the way specs are implemented etc often means the issues aren't that simple, so its means that the implementation is almost never going to be simple.

If you have a look at any window manager, they are often large and complicated beasts, doing lots of things that you wouldn't even expect a window manager to have to do.

However, window managers often start small and grow to accomadate the quirks in their surrounding environment. The best approach, in my opinion, is to encapsulate complexity into clearly defined areas of the code, so that your handing of corner cases doesn't pollute your main buisness logic or get tangled in-between it. While doing that, ensure that the corner cases are tested codepaths that future maintainers can rely upon the tests when making changes.

This is something that I think compiz really succeeds at. While our test coverage is pretty absymal at best, no other non-niche window manager I can think of is as well tested as compiz is. And its starting to pay off - I've been able to rely on the test suite alone for making some changes now, which is something that I was never able to do.

That being said, I think you're right into looking into any more workarounds for the nvidia problems with ConfigureWindow requests and simultaneous redirected GL. We've found the main cause and root of the problem now, so lets stick with this implementation and work on other things. Window movement is 60FPS here with this applied, so I doubt we can make it any faster :)

OK, forget the style issues. I don't want to spend any more hours on testing more revisions.

Sorry, I had to revert this merge in r3528. It caused too many regressions. See bug 1089279.

Ah, I thought there might be some regressions.

Thanks, I'll get on to it.

On Thu, Dec 13, 2012 at 4:50 PM, Daniel van Vugt
<email address hidden> wrote:
> Review: Needs Fixing
>
>
> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.performance_1027211.2/+merge/138800
> Your team Compiz Maintainers is subscribed to branch lp:compiz.

--
Sam Spilsbury