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.

grendello added a commit to grendello/runtime that referenced this pull request Jan 27, 2025
* main: (22 commits)
  Clean up Stopwatch a bit (dotnet#111834)
  JIT: Fix embedded broadcast simd size (dotnet#111638)
  Revert potential UB due to aliasing + more WB removals (dotnet#111733)
  re-enable acceleration of Vector512<long>.op_Multiply (dotnet#111832)
  Handle OSSL 3.4 change to SAN:othername formatting
  JIT: Fix stack allocated arrays for NativeAOT (dotnet#111827)
  JIT: enhance RBO inference for similar compares to constants (dotnet#111766)
  JIT: Don't run optSetBlockWeights when we have PGO data (dotnet#111764)
  [Android] Make sure RuntimeFlavor=CoreCLR when clr subset is specified (dotnet#111821)
  Change empty subject test certificate to include a critical SAN.
  Fix reversed code offsets in GcInfo (dotnet#111792)
  Swap some libraries areas between leads (dotnet#111816)
  Add left-handed spherical and cylindrical billboards (dotnet#109605)
  JIT: revise `optRelopImpliesRelop` to always set `reverseSense` (dotnet#111803)
  Fix Zip64ExtraField handling (dotnet#111802)
  Add build support for Android+CoreCLR (dotnet#110471)
  arm64: Add bic(s) compact encoding (dotnet#111452)
  JIT: Ensure `BBF_PROF_WEIGHT` flag is set when we have PGO data (dotnet#111780)
  Add support for AVX10.2, Add AVX10.2 API surface and template tests (dotnet#111209)
  JIT: Preliminary for enabling inlining late devirted calls (dotnet#111782)
  ...
@TonyValenti
Copy link

Regarding negative values, I THINK I have observed this in the past and attributed it to either a daylight savings time adjustment or windows time synchronization. Maybe those are no longer issues but I wanted to share.

@stephentoub
Copy link
Member Author

It's worth reiterating that this special-casing was only in Stop. It didn't apply if you did:

sw.Start();
...;
TimeSpan elapsed = sw.Elapsed;

only if you did:

sw.Start();
...;
sw.Stop();
TimeSpan elapsed = sw.Elapsed;

That's true on .NET Framework as well.

Given that, even if there are still some machines in circulation where this could manifest, I'm comfortable removing this one place it was being special-cased.

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
6 participants