fix: jit-times - Timestamp arithmetic + add warnings for negative times #9484
Conversation
|
@dotnet-policy-service agree |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
tools/jit-times/Timestamp.cs
Outdated
| public int milliseconds; | ||
| public int nanoseconds; | ||
|
|
||
| public record struct Timestamp(long nanoseconds) : IComparable { |
There was a problem hiding this comment.
While moving to store only nanoseconds within the Unix epoch is a massive simplification, it also reduces the amount of time that can be tracked. As this is "nanoseconds of Unix epoch" in a signed 64-bit integer, the maximum time this approach can hold is around 2262-Apr-11. Any date after 23h47min16sec on that date results in a negative value:
(long) (new DateTime(2262, 4, 11, 23, 47, 17) - DateTime.UnixEpoch).TotalNanoseconds
// -9223372036854775808Given that this is still 238 years away, this probably doesn't matter…
There was a problem hiding this comment.
No disrespect, but if MS folks think it makes sense write this comment to PR + make others read it... I suspect I understand why there is zero progress with some huge issues like the one this PR is related to.
Space ` ` before `(`. Use `1_000_000` instead of `1000_000`. Add `checked()` arithmetic. We don't want Timestamp.operator+() to wrap around to a negative value; that's not useful.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Plus and minus operators for Timestamp type can produce invalid Timestamps. All of Timestamp parts should be either (0 or positive) or (0 or negative), but statements like
result.milliseconds--don't take this value into account in the next "if".The reporting there is broken - the tool parses methods.txt in assumption it is produced by a single-threaded app, and it seems that it's not the case in reality. That's why many "self" timings are negative.
I truncate all negative self-times to zero while calculating total.