Mir

Merge lp://qastaging/~afrantzis/mir/clock-steady-duration-for-timestamp into lp://qastaging/mir

Proposed by Alexandros Frantzis
Status: Rejected
Rejected by: Alexandros Frantzis
Proposed branch: lp://qastaging/~afrantzis/mir/clock-steady-duration-for-timestamp
Merge into: lp://qastaging/mir
Prerequisite: lp://qastaging/~afrantzis/mir/clock-rework
Diff against target: 247 lines (+26/-26)
13 files modified
common-ABI-sha1sums (+1/-1)
include/common/mir/time/types.h (+2/-2)
server-ABI-sha1sums (+1/-1)
src/client/logging/perf_report.cpp (+2/-2)
src/common/symbols.map (+1/-1)
src/common/time/CMakeLists.txt (+1/-1)
src/common/time/steady_clock.cpp (+4/-4)
src/include/common/mir/time/steady_clock.h (+5/-5)
src/server/default_server_configuration.cpp (+2/-2)
tests/include/mir_test_doubles/advanceable_clock.h (+2/-2)
tests/mir_test_doubles/mock_timer.cpp (+1/-1)
tests/unit-tests/graphics/mesa/test_display.cpp (+2/-2)
tests/unit-tests/test_asio_main_loop.cpp (+2/-2)
To merge this branch: bzr merge lp://qastaging/~afrantzis/mir/clock-steady-duration-for-timestamp
Reviewer Review Type Date Requested Status
Alan Griffiths Needs Information
PS Jenkins bot (community) continuous-integration Needs Fixing
Review via email: mp+240436@code.qastaging.launchpad.net

Commit message

time: Use a steady (monotonic) clock

We use mir::time::Clock to calculate intervals, which is dangerous if we use a non-monotonic clock (std::chrono::high_resolution_clock is not guaranteed to be monotonic). This MP changes the code to use a monotonic clock (std::chrono::steady_clock).

A problem with our current approach is that the clock type leaks into our public interface (through the mir::time::Timestamp type definition). To fix this, this MP also changes the Timestamp definition to be a duration from an unspecified epoch. Since the mir::time::Clock::now() is meaningless (and shouldn't be used) as the wall clock time, having a specific, well-known epoch doesn't matter.

Description of the change

time: Use a steady (monotonic) clock

We use mir::time::Clock to calculate intervals, which is dangerous if we use a non-monotonic clock (std::chrono::high_resolution_clock is not guaranteed to be monotonic). This MP changes the code to use a monotonic clock (std::chrono::steady_clock).

A problem with our current approach is that the clock type leaks into our public interface (through the mir::time::Timestamp type definition). To fix this, this MP also changes the Timestamp definition to be a duration from an unspecified epoch. Since the mir::time::Clock::now() is meaningless (and shouldn't be used) as the wall clock time, having a specific, well-known epoch doesn't matter. The drawback is that time::Timestamp is actually a duration type which can be confusing under some circumstances.

Also see the alternative proposal that introduces the steady clock without the fix for the clock type leak: https://code.launchpad.net/~afrantzis/mir/clock-steady/+merge/240439

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

*Needs discussion*

I'm not sure I like either approach.

Here I don't like using time-since-epoch as a timestamp as duration and timestamp are different concepts and should be distinguished by the type system.

On the other branch I don't think we should just be switching from HR clock to steady - both are useful. (And I'm not sure we don't actually need both.)

review: Needs Information
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

The other proposal is more popular, rejecting this.

Unmerged revisions

2021. By Alexandros Frantzis

time: Use a steady (monotonic) clock and make the timestamp clock-independent

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches