ValueStopwatch API suggestion

[WeihanLi]

Background and Motivation

Currently many places are using ValueStopwatch instead of Stopwatch to use reduce allocation, why don't we provide it in the BCL, inspired by https://github.com/dotnet/extensions/blob/master/src/Shared/src/ValueStopwatch/ValueStopwatch.cs

Proposed API

namespace System.Diagnostics
{
+    public class ValueStopwatch {
+        public void Restart();
+        public void Stop();
+        public TimeSpan Elapsed { get; }
+        public static ValueStopwatch StartNew();
}

Usage Examples

var watch = ValueStopwatch.StartNew();
DoSomeWork();
var durationSeconds = watch.Elapsed.TotalSeconds;
watch.Restart();
DoSomeWork();
watch.Stop();
durationSeconds = watch.Elapsed.TotalSeconds;

Possible implement

/// <summary>
/// Value-type replacement for <see cref="Stopwatch"/> which avoids allocations.
/// </summary>
public struct ValueStopwatch
{
    private static readonly double _timestampToTicks = TimeSpan.TicksPerSecond / (double)Stopwatch.Frequency;

    private long _startTimestamp, _stopTimestamp;

    private ValueStopwatch(long startTimestamp)
    {
        _startTimestamp = startTimestamp;
        _stopTimestamp = 0;
    }

    /// <summary>Gets the total elapsed time measured by the current instance.</summary>
    /// <returns>A read-only <see cref="T:System.TimeSpan"></see> representing the total elapsed time measured by the current instance.</returns>
    public TimeSpan Elapsed
    {
        get
        {
            var end = _stopTimestamp > 0 ? _stopTimestamp : Stopwatch.GetTimestamp();
            var timestampDelta = end - _startTimestamp;
            var ticks = (long)(_timestampToTicks * timestampDelta);
            return new TimeSpan(ticks);
        }
    }

    /// <summary>Gets a value indicating whether the <see cref="T:System.Diagnostics.ValueStopwatch"></see> timer is running.</summary>
    /// <returns>true if the <see cref="T:System.Diagnostics.ValueStopwatch"></see> instance is currently running and measuring elapsed time for an interval; otherwise, false.</returns>
    public bool IsRunning => _stopTimestamp == 0;

    /// <summary>Stops time interval measurement, resets the elapsed time to zero, and starts measuring elapsed time.</summary>
    public void Restart()
    {
        _startTimestamp = Stopwatch.GetTimestamp();
        _stopTimestamp = 0;
    }

    /// <summary>Stops measuring elapsed time for an interval.</summary>
    public void Stop() => _stopTimestamp = Stopwatch.GetTimestamp();

    /// <summary>
    /// Creates a new <see cref="ValueStopwatch"/> that is ready to be used.
    /// </summary>
    public static ValueStopwatch StartNew() => new(Stopwatch.GetTimestamp());
}

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[kevingosse]

I think the Restart makes it tricky to use (because of value semantics) and brings little value (why reusing the stopwatch since it's allocated on the stack anyway?).

That's a personal opinion, but I feel like what is really missing is a static helper method that takes two timestamps and converts them to a TimeSpan (basically the same logic as the Elapsed property). Then we should be just fine calling directly Stopwatch.GetTimestamp() where we want to avoid the heap allocation from Stopwatch.

In any case, whether it's ValueStopwatch or some kind of GetElapsed helper, I I strongly believe that there is a need for it. Just looking at the number of times the same logic is reimplemented in this repo (and I myself reiplemented it in many projects): https://github.com/dotnet/runtime/search?q=Stopwatch.Frequency

[WeihanLi]

In some scenarios, we may need to record several times continuously on the stack, so I'm wondering that if we could add Restart method to make it could be reused, but I'm not sure if it'll cause some other problems or not.

[stephentoub]

I prefer @kevingosse's suggestion as well, e.g. something like:

public class Stopwatch
{
    public static TimeSpan GetElapsedTime(long startTimestamp, long endTimestamp) =>
        new TimeSpan((endTimestamp - startTimestamp) * s_frequency);
    ...
}

[davidfowl]

I think we should remove the reset. I'd also love to see how the proposed stopwatch API would be used in place of ValueStopwatch. I think we use this today in a number of places so it would be good to see a before after.

[tannergooding]

Stopwatch itself is largely just a wrapper over the static GetTimestamp method and Frequency field plus some normalization logic to convert it to 100ns ticks (which is what TimeSpan and friends use.

I would guess that the proposed GetElapsedTime method just takes two GetTimestamp queries and does the relevant normalization logic, thus removing the need for an entirely new type, etc.

[davidfowl]

Yep is still like to see some updated places to see if it's as ergonomic as ValueStopWatch.

[stephentoub]

I trust your imagination can extrapolate ;-) Anywhere you store a ValueStopWatch today, you instead just store a long, and anywhere you call GetElapsedTime() today, you instead pass the stored long and Stopwatch.GetTimestamp() to GetElapsedTime(...). If you wanted to save a few characters, you have another overload that's:

public class Stopwatch
{
    public static TimeSpan GetElapsedTime(long startTimestamp) =>
        GetElapsedTime(startTimestamp, Stopwatch.GetTimestamp());
    ...
}

[davidfowl]

I like that overload. I was looking at the places we use it today and it would be a nice drop in replacement to enable passing just the start time.

The other thing to consider is small wrapper structs can be convenient target for extension methods if there's any other functionality we'd like to add. Extension methods on long wouldn't be as nice 😁

[sharwell]

dotnet/roslyn#41205 contains two implementations (one meant to match Stopwatch, and a simpler one we're actually using).

[sharwell]

you instead just store a long

Failure to strongly type the backing value would make the approach no longer a viable replacement for having to implement our own value type replacement for Stopwatch. So at that point, the only thing we save by having the new API is possibly a simpler implementation of SharedStopwatch, except it's already so simple I'm not sure why it would matter.

[stephentoub]

Failure to strongly type the backing value would make the approach no longer a viable replacement for having to implement our own value type replacement for Stopwatch

Why?

[sharwell]

Failure to strongly type the backing value would make the approach no longer a viable replacement for having to implement our own value type replacement for Stopwatch

Why?

It's a use-site constraint related to clarity of code. Two different approaches to resolving a problem are presented above, and I'm saying one of them would have more meaningful downstream impact than the other in the context of one specific downstream project. If the constraint we face in dotnet/roslyn matches the constraint other people have when looking at this proposal, it could be an indicator that the proposal wouldn't serve the intended purpose as well as hoped.

[stephentoub]

related to clarity of code

In other words, it's an eye-of-the-beholder style consideration. That's fine. A ton of code uses Frequency directly, and this would clean those up a bit. If someone wants their own struct for style reasons, it's then a trivial helper on top of these also trivial helpers. I don't see a need to expose a struct from the runtime here.