Merge lp://qastaging/~alan-griffiths/mir/fix-1532202 into lp://qastaging/mir

PASSED: Continuous integration, rev:3251
https://mir-jenkins.ubuntu.com/job/mir-ci/87/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/87/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/87/rebuild

PASSED: Continuous integration, rev:3251
http://jenkins.qa.ubuntu.com/job/mir-ci/6059/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5590
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4497
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5546
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/300
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/383
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/383/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/383
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/383/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5543
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5543/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8002
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26789
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/296
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/296/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/152
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26801

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/6059/rebuild

Sounds like a handy optimization to have. Although I can't help but think we're missing the main issue. The main issue is probably related to the fact that you mentioned the system was mostly idle while we were failing to meet frame deadlines. So it's not necessarily a hard performance limit we hit as much as inefficient blocking/waiting somewhere.

> Sounds like a handy optimization to have. Although I can't help but think
> we're missing the main issue. The main issue is probably related to the fact
> that you mentioned the system was mostly idle while we were failing to meet
> frame deadlines. So it's not necessarily a hard performance limit we hit as
> much as inefficient blocking/waiting somewhere.

Yes, there's an iceberg of issues here. We should return to these and I've logged lp:1535894 to cover that.

Wasn't the issue Unity8? I recall QtMir is full of TODOs for multimonitor support, particularly around correct usage of compositor acquire IDs.

Aside from the above comment, my reason for not liking regional damage is based on previous experience in Compiz and even in mir_proving_server. Accurately tracking damage when you might be painting things using free-form OpenGL (not bound by any surface extents) like shadows is just too much of a pain to maintain. And on low-end systems can become a CPU bottleneck.

Put another way, given two monitors A and B, your window may only exist on A, but you really have to redraw both A and B in case shadows or some other side-effect creeps across.

Yes, QtMir and Unity8 also have work to do. But this issue was explicitly about Mir - and we ought to get this working as well as HWC allows.

Looks good.

No tests?

Another bigger concern is that this branch seems like a crutch. It lets us only just maintain a good frame rate so long as we only need to redraw one monitor. It is however an admission that we're not capable of redrawing multiple monitors per frame, which sounds like a problem that still needs fixing.

Not doing unnecessary rendering isn't "a crutch", especially on a battery powered device.

I don't consider our frame rate problems at an end, but I think this is an increment in the right direction.

It is a crutch if you've established the system is mostly idle and therefore we're not trying very hard to render sufficiently fast. We've had and still do have other such performance problems where it's a matter of real-time getting wasted in blocking system calls, but no CPU or GPU time getting wasted. Bug 1395421 comes to mind.

Adding a little complexity like this to improve performance seems harmless but is almost certainly missing the main issue.

lgtm

Not 100% sure what:
run_cv.notify_one();

Is actually *running* but does it block on this thread? (Since we block at the top of that function).

> lgtm
>
> Not 100% sure what:
> run_cv.notify_one();
>
> Is actually *running* but does it block on this thread? (Since we block at the
> top of that function).

It is an implementation detail of compositor logic: the "run()" function blocks on run_cv awaiting a notification that there's work to do.

lgtm too, apart from having some MultiThreadedCompositor tests

Late test, but improves MM performance on N7