Mir

Merge lp://staging/~kdub/mir/nbs-overallocation into lp://staging/mir

nbs-overallocation
Merge into development-branch

Proposed by Kevin DuBois on 2016-03-23

Status:

Merged

Approved by:

Kevin DuBois on 2016-04-01

Approved revision:

no longer in the source branch.

Merged at revision:

3433

Proposed branch:

lp://staging/~kdub/mir/nbs-overallocation

Merge into:

lp://staging/mir

Diff against target:

280 lines (+136/-25)

4 files modified

src/client/buffer_stream.cpp (+21/-0)
src/client/buffer_vault.cpp (+45/-11)
src/client/buffer_vault.h (+5/-0)
tests/unit-tests/client/test_buffer_vault.cpp (+65/-14)

To merge this branch:

bzr merge lp://staging/~kdub/mir/nbs-overallocation

Related bugs:

Bug #1557442: NBS causes sudden loss of performance after un-fullscreening a swap interval zero client	Medium	Fix Released
Bug #1557962: NBS fullscreen overlay benchmark performance is much lower than BufferQueue	High	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Alan Griffiths			Approve on 2016-04-01
Daniel van Vugt			Abstain on 2016-04-01
Mir CI Bot	continuous-integration		Approve on 2016-03-31
Cemil Azizoglu (community)		2016-03-23	Approve on 2016-03-28
Review via email: mp+289912@code.staging.launchpad.net

Commit message

nbs: add client-side overallocation when there's buffer pressure due to overlay/bypass scenarios and swapinterval is 0.

fixes: LP: #1557962

Description of the change

nbs: add client-side overallocation when there's buffer pressure due to overlay/bypass scenarios and swapinterval is 0.

fixes: LP: #1557962

note: thought we may not have needed, but turns out we do. I suppose this should land before lp:~kdub/mir/nbs-by-default lands.

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-03-23:

^^ seems a problem with the devices

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-03-28:

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-29:

+ if (current_swap_interval == interval)
+ return;

Should current_swap_interval be guarded by "mutex"?

If so, the lock is missing.

If not, then it is unclear which variables should be protected by "mutex".

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-03-30:

This isn't a review, but I suggest more explicit logic to explain the reasons here. Something like:

// Calculate how many concurrent buffers are required to keep all our processes running smoothly in parallel:

int overlaid_buffers = (bypass||overlay) ? 1 : 0;
int compositing_buffers = 1;
int client_buffers = (swap_interval == 0) ? 2 : 1;
int total_buffers_required = client_buffers + compositing_buffers + overlaid_buffers;

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-03-30:

'Needs Information' just to remind me to check this again later.

review: Needs Information

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-03-30:

Or even better:

// Calculate how many concurrent buffers are required to keep all our processes running smoothly in parallel:

int frames_visible_at_once = (nmonitors > 1) ? 2 : 1;
int overlaid_buffers = (bypass||overlay) ? frames_visible_at_once : 0;
int compositing_buffers = frames_visible_at_once;
int client_buffers = (swap_interval == 0) ? 2 : 1;
int min_buffers_required = client_buffers + compositing_buffers + overlaid_buffers;

Because holding an overlay/bypass buffer on the screen for scanout happens in parallel to compositing the next server frame, happens in parallel to the client preparing for the next frame after that.

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-03-30:

@swapinterval, working on fixing.

@more explicit calculation
From the client's perspective, the client just really knows how many buffers the client has or how many the server has. It can increase its allocation count when it runs out (according to a policy), but it doesn't have to know why its increasing its count. In this MP, the policy is simplistic (increase when swapinterval==0), but sufficient for the bug. In the future, a better policy would be to detect periods of buffer pressure and adjust on the fly.

Revision history for this message

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