Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stopwatch is inconsistent when guarding against negative Elapsed durations #66734

Closed
MihaZupan opened this issue Mar 16, 2022 · 6 comments · Fixed by #111834
Closed

Stopwatch is inconsistent when guarding against negative Elapsed durations #66734

MihaZupan opened this issue Mar 16, 2022 · 6 comments · Fixed by #111834
Assignees
Labels
area-System.Runtime bug in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@MihaZupan
Copy link
Member

MihaZupan commented Mar 16, 2022

Stopwatch has the following code to detect negative elapsed durations, but this logic only runs during the Stopwatch.Stop call.

if (_elapsed < 0)
{
// When measuring small time periods the Stopwatch.Elapsed*
// properties can return negative values. This is due to
// bugs in the basic input/output system (BIOS) or the hardware
// abstraction layer (HAL) on machines with variable-speed CPUs
// (e.g. Intel SpeedStep).
_elapsed = 0;
}

If this behavior is worth preserving for stopped Stopwatch instances, I don't see why the responsibility should be shifted to the caller for "active" ones.

I believe this logic should either be removed entirely, or apply to all overloads regardless of IsRunning, possibly including the new GetElapsedTime helpers.

I wouldn't be surprised if some of our own logic is not accounting for the possibility of negative durations.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Mar 16, 2022
@ghost
Copy link

ghost commented Mar 16, 2022

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Stopwatch has the following code to detect negative elapsed durations, but this logic only runs during the Stopwatch.Stop call.

if (_elapsed < 0)
{
// When measuring small time periods the Stopwatch.Elapsed*
// properties can return negative values. This is due to
// bugs in the basic input/output system (BIOS) or the hardware
// abstraction layer (HAL) on machines with variable-speed CPUs
// (e.g. Intel SpeedStep).
_elapsed = 0;
}

If this behavior is worth preserving for stopped Stopwatch instances, I don't see why the responsibility should be shifted to the caller for "active" ones.

I believe this logic should either be removed entirely, or apply to all overloads regardless of IsRunning, possibly including the new GetElapsedTime helpers.

Author: MihaZupan
Assignees: -
Labels:

bug, area-System.Diagnostics

Milestone: -

@tommcdon tommcdon added this to the 7.0.0 milestone Mar 18, 2022
@tommcdon tommcdon removed the untriaged New issue has not been triaged by the area owner label Mar 18, 2022
@stephentoub
Copy link
Member

If this was "due to bugs", is this even still an issue?

I agree we should either delete the logic or make it apply everywhere. It doesn't make sense to only do this for stopped, if it's even needed.

@ghost
Copy link

ghost commented May 8, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Stopwatch has the following code to detect negative elapsed durations, but this logic only runs during the Stopwatch.Stop call.

if (_elapsed < 0)
{
// When measuring small time periods the Stopwatch.Elapsed*
// properties can return negative values. This is due to
// bugs in the basic input/output system (BIOS) or the hardware
// abstraction layer (HAL) on machines with variable-speed CPUs
// (e.g. Intel SpeedStep).
_elapsed = 0;
}

If this behavior is worth preserving for stopped Stopwatch instances, I don't see why the responsibility should be shifted to the caller for "active" ones.

I believe this logic should either be removed entirely, or apply to all overloads regardless of IsRunning, possibly including the new GetElapsedTime helpers.

I wouldn't be surprised if some of our own logic is not accounting for the possibility of negative durations.

Author: MihaZupan
Assignees: -
Labels:

bug, area-System.Runtime

Milestone: 8.0.0

@ericstj
Copy link
Member

ericstj commented May 8, 2023

Adjusted the label on this since Stopwatch lives in System.Runtime.

@tannergooding
Copy link
Member

We've switched to using exclusively monotonic timers for quite a while now and so I think this code is effectively "dead".

We might need to confirm that same is true for Mono, but I believe its mostly all shared atm.

@tannergooding tannergooding modified the milestones: 8.0.0, Future Aug 14, 2023
@tannergooding
Copy link
Member

Moving to future. The implementation is calling exclusively into 64-bit monotonic timers as of right now.

It would be good to remove this now unnecessary logic and document the reasoning. The overflow rate for 1ns ticks is approx. 292 years, which is more than reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime bug in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants