Mir

Merge lp:~alan-griffiths/mir/discussion-point into lp:mir

discussion-point
Merge into development-branch

Proposed by Alan Griffiths on 2016-02-03

Status:	Merged
Approved by:	Cemil Azizoglu on 2016-02-11
Approved revision:	3291
Merged at revision:	3309
Proposed branch:	lp:~alan-griffiths/mir/discussion-point
Merge into:	lp:mir
Diff against target:	15 lines (+2/-2) 1 file modified tests/acceptance-tests/throwback/test_client_cursor_api.cpp (+2/-2)
To merge this branch:	bzr merge lp:~alan-griffiths/mir/discussion-point
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Approve on 2016-02-12
Cemil Azizoglu (community)			Approve on 2016-02-11
Andreas Pokorny (community)			Approve on 2016-02-09
Mir CI Bot	continuous-integration		Approve on 2016-02-09
Daniel van Vugt		2016-02-03	Disapprove on 2016-02-08
Alexandros Frantzis (community)			Needs Information on 2016-02-05
Chris Halse Rogers			Approve on 2016-02-04
Kevin DuBois (community)			Approve on 2016-02-04
Alberto Aguirre			Approve on 2016-02-04
Review via email: mp+284935@code.launchpad.net

Commit Message

tests: Clients don't need to wait for server requests to complete to ensure the requests are ordered.

Description of the Change

Point for discussion: Clients don't need to wait for server requests to complete to ensure the requests are ordered. This arose from a discussion of lp:~alan-griffiths/mir/workaround-1525003 where these changes were incorporated with other minor tidy-up.

Vis: "I'm still not convinced this is theoretically correct. If the "mir_surface_set_state(surface, mir_surface_state_fullscreen);" contributes anything to the test then it will only do so if the request is completed in time. If the surface is not full screen before we do cursor tinkering then either some test may fail, or you didn't need the fullscreening in the first place.

I know that the server presently completes most requests in order, but don't think we document that, or should document that."

Firstly, the mir_wait_for() only ensures that the RPC call completes, it doesn't ensure that the server has taken action.

Secondly requests sent by the client are ordered, so the mir_surface_set_state() call will be received by the server before the mir_surface_configure_cursor() regardless of any mir_wait_for().

That the server processes RPC calls in the order they are received is expected behavior - and would be surprised if we ever change that. Doing so would invalidate other tests - e.g. ClientLibrary.can_set_surface_state.

Mir CI Bot (mir-ci-bot) wrote on 2016-02-03:

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

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

review: Approve (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2016-02-03:

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

review: Approve (continuous-integration)

Daniel van Vugt (vanvugt) wrote on 2016-02-04:

Maybe we could accurately state that ordering is maintained on a per-object basis, or per-method basis right now. Although I am hesitant. It would be nice to reserve the right for the server to be completely asynchronous later (ie. complete calls out of order but as quickly as possible).

Who's to say that there won't be some future "mir_surface_start_something_thats_slow" which takes a long time and may not complete till much later than some methods begun after it?

Buffer swapping and physical display behaviour comes to mind. Imagine this:

wait1 = mir_surface_has_scanned_out(surf); // or something else "slow"
wait2 = mir_surface_update_some_attribute(surf, x);

In this case, wait2 would almost certainly complete first. You don't want to slow down the whole client assuming wait1 must complete first.

That said, your example below is a good case where we would like to avoid all mir_wait calls because they add unnecessary blocking.

review: Abstain

Alan Griffiths (alan-griffiths) wrote on 2016-02-04:

1. There are a lot (e.g. surface reconfiguration) calls in the client API that should be "fire and forget". Because there is no guarantee of how, when or if the server will respond to them - e.g. a requested reconfiguration of a surface may or may not fit with the shell's active policy. Requiring the client to block on each of these calls in order to ensure a consistent ordering would be poor API design[*].

2. The fact that the RPC call completes doesn't guarantee that the server has "handled" the request. (Unless we specify it somewhere it could simply be placed on a queue to be handled later or affect a property that is not currently in effect.)

3. We have existing tests that assume that surface reconfiguration calls are consistently ordered. E.g.

    // Stress-test synchronization logic with some flooding
    for (int i = 0; i < 100; i++)
    {
        mir_surface_set_state(surface, mir_surface_state_maximized);
        mir_surface_set_state(surface, mir_surface_state_restored);
        mir_wait_for(mir_surface_set_state(surface, mir_surface_state_fullscreen));
        ASSERT_THAT(mir_surface_get_state(surface), Eq(mir_surface_state_fullscreen));
    }

Without the guarantee that the third of these mir_surface_set_state() calls is strictly ordered after the preceding two the test would be racy.

[*] In fact changing the return type of these functions to "void" is desirable, but would be an ABI breaking change.

Alberto Aguirre (albaguirre) wrote on 2016-02-04:

LGTM.

review: Approve

Alberto Aguirre (albaguirre) wrote on 2016-02-04:

From a usage perspective and mental model of the client facing api, having requests ordered by submission makes sense.

>Buffer swapping and physical display behaviour comes to mind. Imagine this:
>
> wait1 = mir_surface_has_scanned_out(surf); // or something else "slow"
> wait2 = mir_surface_update_some_attribute(surf, x);
>
>In this case, wait2 would almost certainly complete first.

The server still processed the requests in order.

There are two separate concepts here, processing order of submitted requests and notification of completion.

Like Alan mentioned above, there are various API's that should be one-way requests (client->server, fire-and-forget) in which case completion notification does not make sense.

In the case of the first api, I would not do an "ask" api, but a toggle of "vysnc" events that you receive on the registered surface event handler.

In summary, MirWaitHandle should die...

Kevin DuBois (kdub) wrote on 2016-02-04:

yes, waiting for a call to complete doesn't make sense for calls where we purposefully give no indication of its success.

review: Approve

Chris Halse Rogers (raof) wrote on 2016-02-04:

+1 on defenestrating MirWaitHandle wherever possible.

I think that, when we *are* returning MirWaitHandles, they must have more semantics than “the RPC call has reached the server” (and they mostly do; create_surface, swap_buffers, etc signal when something is ready).

It's easy to ensure that requests are submitted in order, and we should document where we *fail* to submit requests in order, but request completion is indeed something that we absolutely do not and should not guarantee occurs in submission order.

review: Approve

Daniel van Vugt (vanvugt) wrote on 2016-02-05:

"In the case of the first api, I would not do an "ask" api, but a toggle of "vysnc" events that you receive on the registered surface event handler."

We have already discussed that (somewhere), and it's unacceptable for the simple reason that you don't want events waking up an idle app 60 times per second when it should be sleeping. That would be a serious power management regression. So request and wait is the way forward there, and such a wait would take longer than most operations that follow it. So operations can finish out of order and we must not prevent that.

The only problems with MirWaitHandle are:
1. We don't necessitate their destruction by the user, so we keep them around and even have to share them between multiple operations; and
2. The name sounds a bit awkward.

I somewhat agree with the below change in isolation. But in the bigger picture we have to be ready for other operations that will complete out of order. If not using MirWaitHandle then something else.

Chris Halse Rogers (raof) wrote on 2016-02-05:

You'd do a one-shot vsync event request rather than a toggle-on, yes.

In general, I think the replacement for MirWaitHandle should be the extension of the event mechanism we already have - when setting surface attributes and such the response to the request is the associated state change event. NBS will do the same for the swap_buffers equivalent, and so on.

The client ends up being synchronised on state rather than request retirement, which is what they care about anyway.

Alexandros Frantzis (afrantzis) wrote on 2016-02-05:

TL;DR I agree with the behavior we want to provide and that it is intuitive (at least for this particular use case), but I don't think that we provide sufficient guarantees in either documentation or in tests.

@Alan
> 1. There are a lot (e.g. surface reconfiguration) calls in the client API that
> should be "fire and forget". Because there is no guarantee of how, when or if
> the server will respond to them - e.g. a requested reconfiguration of a
> surface may or may not fit with the shell's active policy. Requiring the
> client to block on each of these calls in order to ensure a consistent
> ordering would be poor API design[*].

I agree with the behavior, as long as we make this explicit in the documentation and also provide tests for it.

> 2. The fact that the RPC call completes doesn't guarantee that the server has
> "handled" the request. (Unless we specify it somewhere it could simply be
> placed on a queue to be handled later or affect a property that is not
> currently in effect.)

For a synchronous operation, returning from RPC implies the completion of the operation (regardless of success or failure). As such, it doesn't matter if the server queues the request; it should complete the RPC call only when the operation completes.

> 3. We have existing tests that assume that surface reconfiguration calls are
> consistently ordered. E.g.
>
> // Stress-test synchronization logic with some flooding
> for (int i = 0; i < 100; i++)
> {
> mir_surface_set_state(surface, mir_surface_state_maximized);
> mir_surface_set_state(surface, mir_surface_state_restored);
> mir_wait_for(mir_surface_set_state(surface,
> mir_surface_state_fullscreen));
> ASSERT_THAT(mir_surface_get_state(surface),
> Eq(mir_surface_state_fullscreen));
> }
>
> Without the guarantee that the third of these mir_surface_set_state() calls is
> strictly ordered after the preceding two the test would be racy.

The fact that we have example/test code that *happens* to do things this way (it's not the main purpose of the test to check ordering) doesn't mean that it's the right way to do things, nor that we provide the sufficient guarantees.

@Chris
"It's easy to ensure that requests are submitted in order, and we should document where we *fail* to submit requests in order, but request completion is indeed something that we absolutely do not and should not guarantee occurs in submission order."

Perhaps I am misunderstanding, but if you think that "request completion is indeed something that we absolutely do not and should not guarantee occurs in submission order" then you should be disapproving this MP. This MP assumes that multiple calls to set the state will be completed in the order they are submitted.

review: Needs Information

Daniel van Vugt (vanvugt) wrote on 2016-02-08:

Heh. I was confused by Chris' comments too. They sounded a little contradictory. But I was too... So to reiterate my intentionally semi-contradictory stance:

1. I agree we could guarantee attribute/state changes like the diff below occur in order. The client should/could not need to mir_wait_ on such things.

2. I assert that some other operations explicitly dependent on external factors will need to occur out of order. Like:
A = mir_connection_next_vsync(conn);
B = mir_connection_create_buffer_stream(conn, ...);
That will almost always complete B first. And it would be seriously detrimental to performance to wait for A first, either implicitly or explicitly.

However implying order some of the time is inconsistent and confusing. What we actually need to unify both cases consistently is a better wait API, which would be of the form:

mir_wait_for_any_of(A, B, ...);

inspired by https://msdn.microsoft.com/en-us/library/windows/desktop/ms687025(v=vs.85).aspx and select() :)

Daniel van Vugt (vanvugt) wrote on 2016-02-08:

Hmm, it sounds like I'm disapproving now. :S

What we have already is safer than this proposition, albeit still sub-optimal. We could make it optimal with some new wait API. So we're better off not even suggesting in any Mir release that asynchronous operations occur in order. Unless some special cases get documented.

Indeed, we do have some cases where ordering is presently implied. And needs to be implied because, for example mir_surface_foo(s) only keeps one unique MirWaitHandle for foo in s. But that's precisely why we have mir_wait_for_one(), which is different to mir_wait_for(). Still, that's really just a workaround for the fact that we published an API where the caller is allowed to ignore and never free the MirWaitHandle. So a new API in which the caller gets a unique wait object (and is required to free it) would clear that up.

review: Disapprove

Alan Griffiths (alan-griffiths) wrote on 2016-02-08:

I think there are two (or three?) positions on this:

1. We don't need the waits. The change is fine.
2. We shouldn't need the waits BUT we don't adequately document whether they are needed.
3. I'm not sure if what Chris is trying to say is something else.

In case 2, it could be argued there's a preexisting issue with documenting our guarantees, but that this change doesn't break anything.

Chris Halse Rogers (raof) wrote on 2016-02-08:

So, I think what I'm saying is:

The waits here are pointless, and should be removed. Returning a MirWaitHandle from mir_surface_set_* was a mistake, doesn't provide any value, and (as shown here) encourages incorrect code.

As far as I can tell, the state setting is attempting to guarantee that the cursor will be inside the bounds of the surface by making it fullscreen. In order to do this it needs to wait until it receives an event that the surface is in mir_surface_state_fullscreen, not simply waiting for the RPC call requesting that change to hit the server.

So, a second improvement to the test would be to add a wait-until-fullscreen-event.

Daniel van Vugt (vanvugt) wrote on 2016-02-09:

Hmm, a few people have mentioned that now. At what point did waiting for "mir_surface_set_state" not mean "wait until the state has been set"? Was it (a) always broken, (b) recently broken, or (c) are people confusing scene changes for visible on-screen changes?

Obviously there's a delay in the visual display process. That's fine, and not what I'm talking about. I just mean that:
mir_wait_for(mir_surface_set_state(surface, mir_surface_state_fullscreen));

should mean you have waited for the scene to update the state of that surface (and that does not, has never, meant it's updated as such on screen yet).

Chris Halse Rogers (raof) wrote on 2016-02-09:

There's *never* been any guarantee that after mir_wait_for(mir_surface_state_set(surface, mir_surface_state_fullscreen)) we have mir_surface_get_state(surface) == mir_surface_state_fullscreen.

Furthermore, we cannot in principle give such a guarantee.

The fact that this holds for our test harness is an accident of the harness' triviality.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Alan Griffiths (alan-griffiths) wrote on 2016-02-09:

Wow! I never expected such a diversity of understanding around the team.

From the client side there is a sequence of strictly ordered events:

1. call requesting "fullscreen"
2. call requesting "disabled cursor"

The waits introduce two additional events with the following total ordering:

1. call requesting "fullscreen"
1a. wait for "fullscreen" request expires
2. call requesting "disabled cursor"
2a. wait for "disabled cursor" request expires

Adding the server side, we have the following two sequences:

1. call requesting "fullscreen"
1i. receive "fullscreen" request
1ii. act on "fullscreen" request

2. call requesting "disabled cursor"
2i. receive "disabled cursor" request
2ii. act on "disabled cursor" request

The only guarantee that the test actually needs is that 1ii precedes 2ii.

For the waits to matter, we need the guarantee that 1ii precedes 1a precedes 2 (from which the required ordering follows).

Now, from the above discussion, it is apparent that there is no consensus that there is an ordering between 1a and 1ii. We have three claims:

A. "1 precedes 2 => 1ii precedes 2ii" - so the waits are unnecessary.
B. "1ii precedes 1a" - so the waits are necessary.
C. Neither "1 precedes 2 => 1ii precedes 2ii" nor "1ii precedes 1a" so the waits are pointless and the test is broken.

There is currently no documentation to support either A or B.

The current implementation actually supports both A and B (and we have code that relies on A).

From the discussion, I think that those rejecting A do so mostly because it is undocumented (and they foresee problems documenting which calls are subject to ordering constraints). OTOH A preexisting documentation issue can be addressed separately.

Those rejecting B do so mostly because it is unnecessarily strict and leads to unrealistic client-side expectations that waiting achieves something. (E.g. that "act on "fullscreen" request" is visible to the client as a "fullscreen" state.)

lp:~alan-griffiths/mir/discussion-point updated on 2016-02-09

3291. By Alan Griffiths on 2016-02-09: merge lp:mir

Mir CI Bot (mir-ci-bot) wrote on 2016-02-09:

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

review: Approve (continuous-integration)

Andreas Pokorny (andreas-pokorny) wrote on 2016-02-09:

I agree to the code change.

And I would like to add:
A general guarantee that A is true for every request on every object seems to be inefficient. Regarding B - there are different states of completion, and I believe for a sequence of requests the visibility of completion of i.e. 1ii is only relevant to the server.

Outside of our test setups, do clients have use cases that are composed of sequences of mirclient requests?

review: Approve

PS Jenkins bot (ps-jenkins) wrote on 2016-02-09:

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

review: Approve (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2016-02-09:

FAILED: Continuous integration, rev:3291
http://jenkins.qa.ubuntu.com/job/mir-ci/6237/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5838
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4745
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5794/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/561
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/561/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/561
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/561/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5791
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5791/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8195/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27407

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

review: Needs Fixing (continuous-integration)

Daniel van Vugt (vanvugt) wrote on 2016-02-10:

Indeed this branch is probably harmless by itself. However it implies a dangerous precedent that we don't want to set. That is operations designed as asynchronous will always complete in the order they began.

If you have an asynchronous public API as we do, then you either need to document the special cases that are safe to not wait for, or to not assume any ordering at all.

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-02-10:

I think we are beyond considering this MP only, when we are approving/disapproving.
I am aligned with RAOF's comment : https://code.launchpad.net/~alan-griffiths/mir/discussion-point/+merge/284935/comments/725089
And I think duflu is aligned with that too, with his "vsync" example.... though that got lost in the noise a bit
Ultimately, the users of the mir client API need to be given some guarantees so they can write correct code. MirWaitHandle doesn't seem to fulfill that.

I am assuming mostly the user can "fire and forget". But for a function that depends on a previous function to complete, I am thinking perhaps we should have explicit "wait_for_xxx" functions to provide a guarantee. I am also okay with a MirWaitHandle as long as it doesn't just return after IPC but actually waits for completion of the requested op.

Theoretically, we could serialize the dependencies coming from the app but we'd have to be overly conservative. It's better to leave it to the app explicitly, as it knows best what it intends to do. We just have to enable it with the right wait functions.

"Needs more discussion"

review: Abstain

Chris Halse Rogers (raof) wrote on 2016-02-11:

So, I'm becoming more convinced that we should provide *no* wait API (other than the file-descriptor driven client dispatch branch that I'll dust off).

For sets of changes which should occur at the same time we've got MirSurfaceSpec to make atomic changes, which we plan to extend to cover all changes which the client wants to change atomically.

For everything else we're mostly event driven, and becoming moreso. As clients need to respond to the state they see in events rather than the state they request they already need an event handler. It doesn't seem very useful to provide them with a parallel, less powerful wait-based API that they can't trust.

Hm, that reminds me. I should suggest removing the MirWaitHandles from MirPresentationChain.

Daniel van Vugt (vanvugt) wrote on 2016-02-11:

> Ultimately, the users of the mir client API need to be given some guarantees
> so they can write correct code. MirWaitHandle doesn't seem to fulfill that.

Actually MirWaitHandle and mir_wait_for (waits for all pending operations that use a handle) do provide a guarantee the user can write correct code [1]. The only problem with it is that it's inefficient to use it reliably in the current form :)

[1] I'm also making the bold assumption that this talk about set_state not actually being complete when the server returns completion is either incorrect, or a simple bug we can fix in the server.

Chris Halse Rogers (raof) wrote on 2016-02-11:

While mir_wait_for(mir_surface_set_state()) does indeed wait until the server has processed the request, it's still incorrect code.

There's no guarantee that the state was set to what the client requested, and even if we made that guarantee (by means of some sort of associated return code) we can't guarantee that the state hasn't already been changed to something else by the time mir_wait_for returns.

mir_wait_for(mir_surface_set_state()) is always incorrect code. It's attempting to do something the API does not, and can not, support.

Daniel van Vugt (vanvugt) wrote on 2016-02-11:

Good point. You'd need some kind of mir_surface_(un)lock() for atomicity. Although graphics clients often forget they're sharing the screen with a human and a shell, either of which can change the things the client has set.

So yes, "mir_wait_for(mir_surface_set_state())" is racy to some extent. But it's much less racy than not waiting at all. It will achieve the desired result much more often than not waiting will.

Alan Griffiths (alan-griffiths) wrote on 2016-02-11:

In the context of tests we can know the behaviour of the shell (and that there's no user messing with things the client does).

So arguments referencing these concerns affect only the guarantees we can offer in general, not the validity of tests that can make additional assumptions about the server's behaviour.

There are two guarantees that some people are assuming in this discussion.

1. That calls are actioned before the wait ends.
2. That (apart from buffer swapping) calls are handled in the order they are submitted.

*Both* of these assumptions are true for the current implementation.
*Neither* of these assumptions is AFAIK documented anywhere.

Outside of tests (with known server policy), is either assumption useful to client code?

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-02-11:

Please refer to the email I sent for the key points and actions to be taken as a result of this discussion.

As far as this MP goes...my vote is +1.

Only dissenter is Daniel, but his concerns are about going down the wrong path, not about this particular MP. We can further discuss those points in the email.

So TAing this one.

review: Approve

PS Jenkins bot (ps-jenkins) on 2016-02-12:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

1	=== modified file 'tests/acceptance-tests/throwback/test_client_cursor_api.cpp'
2	--- tests/acceptance-tests/throwback/test_client_cursor_api.cpp 2016-02-03 15:57:44 +0000
3	+++ tests/acceptance-tests/throwback/test_client_cursor_api.cpp 2016-02-09 11:34:21 +0000
4	@@ -468,9 +468,9 @@
5	// for the test logic. - alan_g
6	std::this_thread::sleep_for(std::chrono::milliseconds(20));
7
8	- mir_wait_for(mir_surface_set_state(surface, mir_surface_state_fullscreen));
9	+ mir_surface_set_state(surface, mir_surface_state_fullscreen);
10	auto conf = mir_cursor_configuration_from_name(mir_disabled_cursor_name);
11	- mir_wait_for(mir_surface_configure_cursor(surface, conf));
12	+ mir_surface_configure_cursor(surface, conf);
13	mir_cursor_configuration_destroy(conf);
14	}
15	};