1702521 - Investigate removing TImeStamp::FromSystemTime

Status: NEW

(Core :: mozglue, enhancement)

Description

[Paul Adenot (:padenot)]

Followup after discussion in bug 1685502, copy/pasting the important points, but background available in the original review thread:

In D101036#3353780, @padenot wrote:

That broke tons of things, because different platforms require different TimeStamp semantics, because we want TimeStamp to be able to be constructed from a "raw" integer value from a system API.

Actually TimeStamp's original design explicitly steered away from that and there was no way to construct an object from a system timer. I went to great lengths to maintain that property when I added uptime computations by completely burying them into the class so that one would not need to pass external timers into it. The reasoning is always the same: mixing different timebases is a nightmare and leads to all sorts of bugs and we didn't want to do that. We added an escape hatch for Firefox OS (sigh) in bug 1083530 that allowed to create a TimeStamp from system time, and then generalized that mechanism in bug 552020. We knew that would have cause trouble down the line and indeed it has.

It seems like there is two types of users of this static method:

• Vsync
• Input events

The pattern is the same: a system API calls a callback, passing an integer in, and it appears that the time base necessary in those call sites match the internal representation of the respective TimeStamp on each platform (roughly, it's a bit unclear on Windows). It's unclear if it's the same clock domain sometimes, but generally it's correct.

This (among other things) prevents changing the implementation of TimeStamp, which would be desirable, especially on Windows, because it's a bit old and slow (new system facilities are available), and more importantly, doesn't have the same semantic as all others (it ticks when the machine is suspended).

What are the thoughts of people involved here? I'm cc-ing based on authorship, but feel free to CC more people.

Comment 1

[Robert Mader [:rmader]]

I don't fully understand yet - what would be the alternatives left to translate a system time hint? Would we always need to use TimeStamp::Now()? That would be unfortunate for vsync and potentially result in much stronger frame jitter on certain platforms.

Comment 2

[Markus Stange [:mstange]]

(In reply to Paul Adenot (:padenot) from comment #0)

What are the thoughts of people involved here?

Well, we have those two access points where the macOS gives us a system timestamp. We need an accurate representation of that timestamp as a mozilla::TimeStamp. That's just the reality of it. And since mixing times from different time bases is a recipe for trouble (as glandium says), making the internal representation of TimeStamp be the same format + timebase as the "native time stamps" seems like the most obvious solution to me, and it's what we have today.

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Karl is better person to answer this question.

Comment 4

[Karl Tomlinson (:karlt)]

On systems where the OS uses multiple kinds of timestamps, some conversion is likely necessary because there is no single reference time suitable for all occasions.

For X11 and WINNT ports, user event times are converted to mozilla::TimeStamp. The code is fairly complicated, there have been subtle difficult-to-reproduce bugs, the conversion isn't always ideal, but transient problems do seem to resolve.

On WINNT, mozilla::Timestamp itself is an excellent example of what can go wrong when mixing time sources.

(In reply to Markus Stange [:mstange] from comment #2)

We need an accurate representation of that timestamp as a mozilla::TimeStamp. That's just the reality of it.

I understand the need to convert system user event times to mozilla::TimeStamp because these are exposed to content for comparison with other timestamps such as those from Performance that are not from system user event times. On X11 and WINNT, the user event times are low res anyway, so imperfect conversion is not a major issue.

I know less about the needs of vsync, but can imagine it could be at least more convenient to work with a conversion to mozilla::Timestamp for XP code, rather than having to consistently use a particular OS-specific time source for all vsync-related code.

And since mixing times from different time bases is a recipe for trouble (as glandium says), making the internal representation of TimeStamp be the same format + timebase as the "native time stamps" seems like the most obvious solution to me, and it's what we have today.

It is certainly the simplest, so there would need to be a good reason to do otherwise.

(In reply to Paul Adenot (:padenot) from comment #0)

This (among other things) prevents changing the implementation of TimeStamp, which would be desirable, especially on Windows, because it's a bit old and slow (new system facilities are available), and more importantly, doesn't have the same semantic as all others (it ticks when the machine is suspended).

The WINNT port does not have Timestamp::FromSystemTime() AFAICS.

Comment 5

[Paul Adenot (:padenot)]

(In reply to Karl Tomlinson (:karlt) from comment #4)

(In reply to Paul Adenot (:padenot) from comment #0)

This (among other things) prevents changing the implementation of TimeStamp, which would be desirable, especially on Windows, because it's a bit old and slow (new system facilities are available), and more importantly, doesn't have the same semantic as all others (it ticks when the machine is suspended).

The WINNT port does not have Timestamp::FromSystemTime() AFAICS.

Yes, ideally we'd want Windows to not tick when the machine is suspended, but input to this conversion code is using time that ticks when the machine is suspended, and TimeStamp works like that on Windows.

Comment 6

[Gabriele Svelto [:gsvelto]]

I haven't had time to look at all the callsites involved but there's a few things that come to mind:

• We can't mix fuzzed and non-fuzzed timers which we're currently doing (and which most likely is leading to real bugs)
• TimeStamp implementations must be consistent, either they all count standby time or they all don't
• We most likely need two different implementations for code that cares about standby time and for code that doesn't and we must make sure we can't mix them
• The Windows TimeStamp needs an overhaul, but that's not possible until we've established different classes for different usages

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Paul Adenot (:padenot) from comment #5)

Yes, ideally we'd want Windows to not tick when the machine is suspended, but input to this conversion code is using time that ticks when the machine is suspended, and TimeStamp works like that on Windows.

Argh. A 32-bit millisecond counter including suspend time is going to be broken, but I guess we have to go with what we have.

SystemTimeConverter is meant to be able to correct for a 32-bit time including suspend getting ahead of a TimeStamp that does not include suspend time. I'm curious to hear if that would not work.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to Gabriele Svelto [:gsvelto] from comment #6)

• TimeStamp implementations must be consistent, either they all count standby time or they all don't
• We most likely need two different implementations for code that cares about standby time and for code that doesn't and we must make sure we can't mix them

I'm all for making it difficult to accidentally mix different time sources.

Many clients just need a monotonic time and don't care whether the standby time effect differs across platforms. Perhaps there are both clients that want standby time included and those that want it excluded, but it could be that the most accurate and low overhead time source is different on different platforms and I expect an inter-platform-inconsistent source can be considered in choosing the best source for many clients.

Comment 9

[Gabriele Svelto [:gsvelto]]

(In reply to Karl Tomlinson (:karlt) from comment #8)

Many clients just need a monotonic time and don't care whether the standby time effect differs across platforms. Perhaps there are both clients that want standby time included and those that want it excluded, but it could be that the most accurate and low overhead time source is different on different platforms and I expect an inter-platform-inconsistent source can be considered in choosing the best source for many clients.

Makes sense so long that users know that they have no specific guarantees from the class ("this is just monotonic, it can't be mixed with timestamps from system events, it can't be used to measure real time accurately").