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

Clean up Stopwatch a bit #111834

Merged
merged 2 commits into from
Jan 26, 2025
Merged

Clean up Stopwatch a bit #111834

merged 2 commits into from
Jan 26, 2025

Conversation

stephentoub
Copy link
Member

  • Remove unnecessary Reset call from ctor
  • Remove unnecessary branch for _elapsed < 0 (Stop is now more inlineable)
  • Remove some defunct comments
  • Clean up style of Stop to match that of Start

Closes #66734
Related to #111829

Method Toolchain Mean Ratio Code Size Allocated Alloc Ratio
Time \main\corerun.exe 38.51 ns 1.00 363 B 40 B 1.00
Time \pr\corerun.exe 30.35 ns 0.79 156 B - 0.00
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Diagnostics;
using System.Runtime.CompilerServices;

BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

[DisassemblyDiagnoser]
[MemoryDiagnoser(false)]
public partial class Tests
{
    [Benchmark]
    public TimeSpan Time()
    {
        Stopwatch sw = Stopwatch.StartNew();
        Nop();
        sw.Stop();
        return sw.Elapsed;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static void Nop() { }
}

- Remove unnecessary Reset call from ctor
- Remove unnecessary branch for _elapsed < 0
- Remove some defunct comments
- Clean up style of Stop to match that of Start
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 26, 2025
@stephentoub stephentoub added area-System.Diagnostics and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jan 26, 2025
@stephentoub stephentoub requested a review from MihaZupan January 26, 2025 02:00
@paulbartrum
Copy link

Are the Debug.Assert(IsHighResolution); asserts still needed? These will always be true as far as I can tell.

- Use expression-bodied properties
- Remove unnecessary Debug.Asserts
- Remove private method whose impl could just have been that of an existing public property
- Change a private GetXx method to an Xx property.
- Remove some type names that could be inferred
@stephentoub
Copy link
Member Author

Are the Debug.Assert(IsHighResolution); asserts still needed? These will always be true as far as I can tell.

They're not needed.

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@stephentoub
Copy link
Member Author

/ba-g unrelated infra

@stephentoub stephentoub merged commit f41b65f into dotnet:main Jan 26, 2025
140 of 142 checks passed
@stephentoub stephentoub deleted the stopwatchstop branch January 26, 2025 18:09
Comment on lines -61 to -70
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;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we still support CPUs that could have this issue on Linux, although such processors are increasingly rare.

The Intel Architecture Manual covers that RDTSC is tied to the processor frequency for Pentium M processors (family [06H], models [09H, 0DH]); for Pentium 4 processors, Intel Xeon processors
(family [0FH], models [00H, 01H, or 02H]); and for P6 family processors

For a constant rate that is not tied to the CPU frequency, you instead need: Pentium 4 processors, Intel Xeon processors (family [0FH], models [03H and higher]); for Intel Core Solo
and Intel Core Duo processors (family [06H], model [0EH]); for the Intel Xeon processor 5100 series and Intel
Core 2 Duo processors (family [06H], model [0FH]); for Intel Core 2 and Intel Xeon processors (family [06H],
DisplayModel [17H]); for Intel Atom processors (family [06H], DisplayModel [1CH]).

But even more so, it is technically dependent on querying that the CPU supports Invariant TSC by querying CPUID 0x8000_0007 and then reading EDX bit 8.

QPC on Windows and CLOCK_MONOTONIC on Unix is supposed to normalize this, but historically there have been bugs.

Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stopwatch is inconsistent when guarding against negative Elapsed durations
5 participants