1841352 - TimeStamp comparisons are expensive and can be incorrect on Windows

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Justin Link]

Comparisons between TimeStamps are implemented as comparisons between TimeStampValues.

On Windows, TimeStampValues are classes and they implement comparisons as a subtraction of two TimeStampValues which is then cast to an int64_t and compared to 0. Subtraction of TimeStampValues is implemented primarily by a function called CheckQPC which ... does a lot of stuff.

In addition to be unexpectedly expensive in terms of execution time, this can produce the wrong result when the difference between the two TimeStamps is larger than can be represented by an int64_t.

Comment 1

[Justin Link]

Attachment: Bug 1841352: Make TimeStamp comparisons actually just be comparisons r=florian

Comment 2

[Justin Link]

Attachment: Bug 1841352: Added (compile-time) unit tests for TimeStamp and TimeStampValue r=florian

Depends on D182675

Comment 3

[Masatoshi Kimura [:emk]]

Is this immune to wrapping around of timestamp values?

Comment 4

[Justin Link]

@Masatoshi: I'm not sure that I understand your question. Can you clarify?

Comment 5

[Masatoshi Kimura [:emk]]

Do we assume that TimeStampValue will never wrap around?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a122c986749c
Make TimeStamp comparisons actually just be comparisons r=florian
https://hg.mozilla.org/integration/autoland/rev/6abbc4528080
Added (compile-time) unit tests for TimeStamp and TimeStampValue r=florian

Comment 7

[Justin Link]

(In reply to Masatoshi Kimura [:emk] from comment #5)

Do we assume that TimeStampValue will never wrap around?

I think the answer to that is yes. But I guess it depends on what you mean by "we", what you mean by "assume" and what you mean by "wrap around".

Timestamps are unsigned 64-bit values internally and that range is expected to be large enough such that we are not going to reach the point in time specified by the maximum value. So actual overflow/wrap-around of real time is not expected to be encountered and I doubt that any users of the TimeStamp class are expecting that kind of overflow to happen. The functionality for getting a TimeStamp representing the current time certainly does not worry about the possibility of time actually wrapping around. (What would it do anyway?)

New TimeStamps can also be created by adding or subtracting a TimeDuration from an existing TimeStamp. This could certainly lead to overflow or underflow. There is no attempt to handle overflow and there is an attempt to handle underflow although, at least prior to this change, it think it wasn't working correctly on Windows due to the previously-mentioned bugs in comparison between TimeStamps that are far apart.

Comment 8

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/a122c986749c
https://hg.mozilla.org/mozilla-central/rev/6abbc4528080