Mir

[regression] [nonblockswap] BufferConsumingFunctor holds buffers too long, potentially hurting client render throughput

Bug #1308844 reported by Daniel van Vugt
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mir
Fix Released
Medium
Mir development team
Mir 0.3.0
mir (Ubuntu)
Fix Released
Medium
Unassigned

Bug Description

[regression] Multi-threaded compositor holds buffers too long, potentially hurting client render throughput.

Until recently the compositor only held client buffers as long as it needed them to render and swapbuffers (a couple of milliseconds). However the recent introduction of BufferConsumingFunctor (r1545) means we're holding each buffer for a full 16ms.

We might not see any visible regression on screen yet, but it's a theoretical problem we should address for low-end hardware performance.

See original description

Related branches

lp:~vanvugt/mir/no-savings
Rejected for merging into lp:mir
PS Jenkins bot (community): Approve (continuous-integration)
Robert Carr (community): Needs Information
Alexandros Frantzis (community): Disapprove
Mir development team: Pending requested
Diff: 19 lines (+1/-3)
1 file modified
src/server/compositor/multi_threaded_compositor.cpp (+1/-3)
lp:~vanvugt/mir/judder
On hold for merging into lp:mir
PS Jenkins bot (community): Needs Fixing (continuous-integration)
Alexandros Frantzis: Pending requested
Alan Griffiths: Pending requested
Mir development team: Pending requested
Diff: 297 lines (+166/-35)
3 files modified
src/server/compositor/multi_threaded_compositor.cpp (+29/-26)
tests/acceptance-tests/test_client_surface_swap_buffers.cpp (+3/-4)
tests/unit-tests/compositor/test_multi_threaded_compositor.cpp (+134/-5)
lp:~raof/mir/1hz-rendering-always
PS Jenkins bot (community): Approve (continuous-integration)
Alan Griffiths: Abstain
Daniel van Vugt: Abstain
Andreas Pokorny (community): Approve
Alexandros Frantzis (community): Abstain
Alberto Aguirre (community): Approve
Diff: 3043 lines (+1552/-378)
38 files modified
include/platform/mir/graphics/buffer_id.h (+1/-1)
include/server/mir/asio_main_loop.h (+2/-0)
include/server/mir/compositor/frame_dropping_policy.h (+69/-0)
include/server/mir/compositor/frame_dropping_policy_factory.h (+57/-0)
include/server/mir/default_server_configuration.h (+3/-0)
include/server/mir/time/timer.h (+8/-1)
include/test/mir_test/fake_clock.h (+72/-0)
include/test/mir_test/signal.h (+63/-0)
include/test/mir_test_doubles/mock_frame_dropping_policy_factory.h (+83/-0)
include/test/mir_test_doubles/mock_timer.h (+52/-0)
include/test/mir_test_doubles/stub_frame_dropping_policy_factory.h (+58/-0)
src/server/asio_main_loop.cpp (+23/-9)
src/server/compositor/CMakeLists.txt (+1/-0)
src/server/compositor/buffer_queue.cpp (+37/-1)
src/server/compositor/buffer_queue.h (+5/-1)
src/server/compositor/buffer_stream_factory.cpp (+6/-4)
src/server/compositor/buffer_stream_factory.h (+4/-4)
src/server/compositor/default_configuration.cpp (+15/-1)
src/server/compositor/multi_threaded_compositor.cpp (+53/-135)
src/server/compositor/timeout_frame_dropping_policy_factory.cpp (+92/-0)
src/server/compositor/timeout_frame_dropping_policy_factory.h (+56/-0)
tests/acceptance-tests/test_client_surface_swap_buffers.cpp (+8/-13)
tests/integration-tests/compositor/test_buffer_stream.cpp (+45/-15)
tests/integration-tests/compositor/test_swapping_swappers.cpp (+3/-1)
tests/integration-tests/graphics/android/test_buffer_integration.cpp (+4/-1)
tests/integration-tests/graphics/android/test_internal_client.cpp (+2/-1)
tests/mir_test/CMakeLists.txt (+2/-0)
tests/mir_test/fake_clock.cpp (+53/-0)
tests/mir_test/signal.cpp (+46/-0)
tests/mir_test_doubles/CMakeLists.txt (+2/-0)
tests/mir_test_doubles/mock_frame_dropping_policy_factory.cpp (+70/-0)
tests/mir_test_doubles/mock_timer.cpp (+149/-0)
tests/unit-tests/compositor/CMakeLists.txt (+1/-0)
tests/unit-tests/compositor/test_buffer_queue.cpp (+128/-46)
tests/unit-tests/compositor/test_multi_threaded_compositor.cpp (+15/-78)
tests/unit-tests/compositor/test_timeout_frame_dropping_policy.cpp (+186/-0)
tests/unit-tests/graphics/mesa/test_linux_virtual_terminal.cpp (+1/-0)
tests/unit-tests/test_asio_main_loop.cpp (+77/-66)
lp:~afrantzis/mir/consume-only-not-rendered-buffers
Rejected for merging into lp:mir
PS Jenkins bot (community): Approve (continuous-integration)
Daniel van Vugt: Needs Fixing
Kevin DuBois (community): Needs Fixing
Alan Griffiths: Needs Information
Diff: 365 lines (+82/-31)
11 files modified
examples/render_overlays.cpp (+11/-1)
include/platform/mir/graphics/renderable.h (+2/-0)
include/test/mir_test_doubles/fake_renderable.h (+10/-1)
include/test/mir_test_doubles/mock_renderable.h (+1/-0)
include/test/mir_test_doubles/stub_renderable.h (+13/-0)
src/server/compositor/multi_threaded_compositor.cpp (+23/-27)
src/server/scene/basic_surface.cpp (+8/-0)
src/server/scene/basic_surface.h (+3/-0)
src/server/scene/surface_stack.cpp (+3/-0)
tests/unit-tests/compositor/test_multi_threaded_compositor.cpp (+2/-2)
tests/unit-tests/graphics/android/test_hwc_device.cpp (+6/-0)
lp:~mir-team/mir/10Hz-rendering-always
PS Jenkins bot (community): Approve (continuous-integration)
Daniel van Vugt: Disapprove
Chris Halse Rogers: Approve
Kevin DuBois (community): Approve
Alexandros Frantzis (community): Approve
Diff: 2890 lines (+1520/-333)
38 files modified
include/platform/mir/graphics/buffer_id.h (+1/-1)
include/server/mir/asio_main_loop.h (+2/-0)
include/server/mir/compositor/frame_dropping_policy.h (+69/-0)
include/server/mir/compositor/frame_dropping_policy_factory.h (+57/-0)
include/server/mir/default_server_configuration.h (+3/-0)
include/server/mir/time/timer.h (+8/-1)
include/test/mir_test/fake_clock.h (+72/-0)
include/test/mir_test/signal.h (+63/-0)
include/test/mir_test_doubles/mock_frame_dropping_policy_factory.h (+83/-0)
include/test/mir_test_doubles/mock_timer.h (+52/-0)
include/test/mir_test_doubles/stub_frame_dropping_policy_factory.h (+58/-0)
src/server/asio_main_loop.cpp (+20/-6)
src/server/compositor/CMakeLists.txt (+1/-0)
src/server/compositor/buffer_queue.cpp (+54/-3)
src/server/compositor/buffer_queue.h (+6/-1)
src/server/compositor/buffer_stream_factory.cpp (+6/-4)
src/server/compositor/buffer_stream_factory.h (+4/-4)
src/server/compositor/default_configuration.cpp (+15/-1)
src/server/compositor/multi_threaded_compositor.cpp (+53/-135)
src/server/compositor/timeout_frame_dropping_policy_factory.cpp (+92/-0)
src/server/compositor/timeout_frame_dropping_policy_factory.h (+56/-0)
tests/acceptance-tests/test_client_surface_swap_buffers.cpp (+8/-13)
tests/integration-tests/compositor/test_buffer_stream.cpp (+44/-14)
tests/integration-tests/compositor/test_swapping_swappers.cpp (+3/-1)
tests/integration-tests/graphics/android/test_buffer_integration.cpp (+4/-1)
tests/integration-tests/graphics/android/test_internal_client.cpp (+2/-1)
tests/mir_test/CMakeLists.txt (+2/-0)
tests/mir_test/fake_clock.cpp (+53/-0)
tests/mir_test/signal.cpp (+46/-0)
tests/mir_test_doubles/CMakeLists.txt (+2/-0)
tests/mir_test_doubles/mock_frame_dropping_policy_factory.cpp (+70/-0)
tests/mir_test_doubles/mock_timer.cpp (+149/-0)
tests/unit-tests/compositor/CMakeLists.txt (+1/-0)
tests/unit-tests/compositor/test_buffer_queue.cpp (+130/-47)
tests/unit-tests/compositor/test_multi_threaded_compositor.cpp (+15/-78)
tests/unit-tests/compositor/test_timeout_frame_dropping_policy.cpp (+186/-0)
tests/unit-tests/graphics/mesa/test_linux_virtual_terminal.cpp (+1/-0)
tests/unit-tests/test_asio_main_loop.cpp (+29/-22)
Daniel van Vugt (vanvugt)
Changed in mir:
assignee: nobody → Daniel van Vugt (vanvugt)
status: Triaged → In Progress
Daniel van Vugt (vanvugt)
description: updated
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

"BufferConsumingFunctor (r1545) means we're holding each buffer for a full 16ms."

Correction, the buffer will be held in the WORST CASE 16ms (just as with an actual display that just barely missed its vsync timeline).

The vysnc wait done in BufferConsumingFunctor emulates a running vsync clock not a full 16ms wait.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

And once it syncs up the first time, with constant frames scheduled it will indefinitely be sync'd up and all frames thereafter sleep for 16ms. So almost always 16ms.

The dynamic timing adjustment in BufferConsumingFunctor appears to be of no value for this reason. You would get the same result with a simple sleep call.

kevin gunn (kgunn72)
summary: - [regression] BufferConsumingFunctor holds buffers too long, potentially
- hurting client render throughput
+ [regression] [nonblockswap] BufferConsumingFunctor holds buffers too
+ long, potentially hurting client render throughput
tags: added: nonblockswap
Revision history for this message
kevin gunn (kgunn72) wrote :

As discussed in the stand-up today.
There's no ill effect seen from a end user point of view on the phone, tablet or xmir.
Based on that, we're accepting this bug as part of the 019 milestone in order to deliver the overall improvement of unblocking events on the phone/tablet.

As for addressing this bug, the team seems to be in agreement on using Chris' proposed solution as an improvement (solving in switching bundle) and a way to correct this issue.
https://code.launchpad.net/~raof/mir/1hz-rendering-always/+merge/216246

kevin gunn (kgunn72)
Changed in mir:
milestone: 0.1.9 → none
milestone: none → 0.1.10
Daniel van Vugt (vanvugt)
Changed in mir:
assignee: Daniel van Vugt (vanvugt) → Mir development team (mir-team)
Daniel van Vugt (vanvugt)
tags: added: regression
removed: regression-update
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

Fix committed into lp:mir/devel at revision None, scheduled for release in mir, milestone Unknown

Changed in mir:
status: In Progress → Fix Committed
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Fix reverted in r1653 due to more serious regressions.

Changed in mir:
status: Fix Committed → In Progress
Cemil Azizoglu (cemil-azizoglu)
Changed in mir:
milestone: 0.2.0 → 0.3.0
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

Fix committed into lp:mir/devel at revision 1675, scheduled for release in mir, milestone Unknown

Changed in mir:
status: In Progress → Fix Committed
Alberto Aguirre (albaguirre)
Changed in mir:
status: Fix Committed → Fix Released
Changed in mir (Ubuntu):
status: New → Fix Released
Daniel van Vugt (vanvugt)
Changed in mir (Ubuntu):
importance: Undecided → Medium
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.