Skip to content

Introducing Environment.ProcessorUserTime & ProcessorPrivilegedTime  #104844

Closed

Description

@tarekgh

We are exposing runtime metrics to report statistics for GC, thread pool, JIT, lock contention, timer, and process time. The implementation of these runtime metrics will reside in the System.Diagnostics.DiagnosticSource library, where the Metrics feature is housed. However, to report ProcessorUserTime and ProcessorPrivilegedTime, we need to call Process.GetCurrentProcess().ProcessorUserTime and Process.GetCurrentProcess().ProcessorPrivilegedTime, which requires adding a dependency on the System.Diagnostics.Process library. This dependency introduces two problems:

  • Cyclic Dependency: System.Diagnostics.DiagnosticSource is a low-level library, and adding a dependency on System.Diagnostics.Process creates a cyclic dependency chain: System.IO.Pipes > System.Net.Sockets > System.Net.NameResolution > System.Diagnostics.DiagnosticSource > System.Diagnostics.Process > System.IO.Pipes.
  • Size Regression: Adding more dependencies to this low-level library will increase the size of AOT builds.

To avoid these issues, we propose exposing new properties in the Environment class to report the same information, thus eliminating the need for the System.Diagnostics.Process dependency.

Proposal

static class Environment
{
    ...
    public static ProcessCpuUsage CpuUsage { get; }
    ...
}

public readonly struct ProcessCpuUsage
{
    public TimeSpan UserTime { get; } // report TimeSpans that are identical to calling Process.GetCurrentProcess().ProcessorUserTime
    public TimeSpan PrivilegedTime { get; } // report TimeSpans that are identical to Process.GetCurrentProcess().ProcessorPrivilegedTime
}

Alternative Design

static class Environment
{
    // identical tocalling Process.GetCurrentProcess().ProcessorUserTime
    public static TimeSpan ProcessorUserTime { get; }

    // identical tocalling Process.GetCurrentProcess().ProcessorPrivilegedTime
    public static TimeSpan  ProcessorPrivilegedTime { get; }
    ...
}

Example

     ProcessCpuUsage processCpuUsage = Environment.CpuUsage.
     Console.WriteLine($"ProcessorUserTime: {processCpuUsage.UserTime}  ... ProcessorPrivilegedTime: {processCpuUsage.PrivilegedTime} ");
     
     // Or the alternative design will be 
     Console.WriteLine($"ProcessorUserTime: {Environment.ProcessorUserTime}  ... ProcessorPrivilegedTime: {Environment.ProcessorPrivilegedTime} ");

Activity

  1. self-assigned this
    on Jul 13, 2024
  2. added this to the 9.0.0 milestone on Jul 13, 2024
  3. dotnet-policy-service commented on Jul 13, 2024

    @dotnet-policy-service
    Contributor

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

  4. added
    api-ready-for-reviewAPI is ready for review, it is NOT ready for implementation
    blockingMarks issues that we want to fast track in order to unblock other important work
    on Jul 13, 2024
  5. dotnet-policy-service commented on Jul 15, 2024

    @dotnet-policy-service
    Contributor

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

  6. terrajobst commented on Jul 15, 2024

    @terrajobst
    Contributor

    Would the alternative design even work? Presumably these two values are linked so one would want to query them atomically, no?

  7. tarekgh commented on Jul 16, 2024

    @tarekgh
    MemberAuthor

    @terrajobst

    The alternative design is like what we are currently exposing from the Process class. But you have a good point as I expect more scenarios will need to snap the user and privileged time together.

    https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process.userprocessortime?view=net-8.0
    https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process.privilegedprocessortime?view=net-8.0

  8. MichalPetryka commented on Jul 16, 2024

    @MichalPetryka
    Contributor

    Is there a reason for separate type instead of a tuple?

  9. stephentoub commented on Jul 16, 2024

    @stephentoub
    Member

    Is there a reason for separate type instead of a tuple?

    We could never add anything else to a tuple.

  10. terrajobst commented on Jul 16, 2024

    @terrajobst
    Contributor

    Video

    • We'd prefer to have a nested type, as opposed to putting it in the System namespace
    • Might as well also expose TotalTime
    namespace System;
    
    public partial class Environment
    {
        [SupportedOSPlatform("maccatalyst")]
        [UnsupportedOSPlatform("ios")]
        [UnsupportedOSPlatform("tvos")]
        public static ProcessCpuUsage CpuUsage { get; }
    
        public readonly struct ProcessCpuUsage
        {
            public TimeSpan UserTime { get; }
            public TimeSpan PrivilegedTime { get; }
            public TimeSpan TotalTime => UserTime + PrivilegedTime;
        }
    }
  11. added
    api-approvedAPI was approved in API review, it can be implemented
    and removed
    api-ready-for-reviewAPI is ready for review, it is NOT ready for implementation
    on Jul 16, 2024
  12. terrajobst commented on Jul 18, 2024

    @terrajobst
    Contributor

    @bartonjs @stephentoub

    @tarekgh asked me whether the struct members should be init too.

    In general, I think we should avoid pure data holder types that can't be constructed as it makes testing harder, which means we either need to add a ctor or make the properties settable. For pure data holders I'd prefer settable props over a constructor because it means we don't need to add a new ctor whenever we add a new data member.

    Now, since we don't like mutable structs we'd probably want init over set. Any objections to this?

    namespace System;
    
    public partial class Environment
    {
        public readonly struct ProcessCpuUsage
        {
            public TimeSpan UserTime { get; init; }
            public TimeSpan PrivilegedTime { get; init; }
            public TimeSpan TotalTime => UserTime + PrivilegedTime;
        }
    }
  13. jkotas commented on Jul 18, 2024

    @jkotas
    Member

    We have number of types in BCL like this one that cannot be constructed. I do not think it makes the testing harder. These types are not exchange types and you cannot hook the api that produce them using DI.

    I think we should stick with the approved surface. If we think that it makes sense to make types like this one constructable, it should be deeper discussion and the pattern applied consistently to all similar types.

  14. stephentoub commented on Jul 18, 2024

    @stephentoub
    Member

    I agree with Jan. If we want it to be constructable, we can later add a ctor or make the properties settable. I don't think init buys much here. We were also strongly considering using a tuple here, which is mutable, so immutability isn't a key requirement.

  15. tarekgh commented on Jul 18, 2024

    @tarekgh
    MemberAuthor

    I will use internal init; for now and if we decide to make it public later, we can do it.

  16. terrajobst commented on Jul 19, 2024

    @terrajobst
    Contributor

    @jkotas @stephentoub

    I'm surprised by this pushback. What flexibility do we gain by not having this struct be constructable? My understanding was that the reason we added this type in the first place was to return user and kernel time as a single unit; that seems to presuppose that our code merely populates this struct with the result, rather than doing anything clever.

    To be clear, I don't particularly care for this struct, just trying to understand our rationale here.

  17. stephentoub commented on Jul 19, 2024

    @stephentoub
    Member

    @jkotas @stephentoub

    I'm surprised by this pushback. What flexibility do we gain by not having this struct constructable? My understanding was that reason we added this type in the first hand is to return user and kernel time as a single unit; that seems to presuppose that our code merely constructs this struct with the result, rather than doing anything clever.

    I'm fine with it being constructable. My pushback is on use of init here, which just seems unnecessarily "cute" (init presumes that the only valid way to create one of these is with object initializer syntax, which is constraining and forces a particular style, in this case unnecessarily). Either we don't want others to construct it, in which case the right shape is the one that was approved, or we do, in which case just make the properties settable (or add a constructor).

  18. jkotas commented on Jul 20, 2024

    @jkotas
    Member

    My concern is about consistency. There are other similar types. GCMemoryInfo, GCGenerationInfo or ProcessModule are some examples that come to my mind. What should be the framework design rule for these types? Should we make all of them initiable with custom values just because of we can? Personally, I do not see the value of doing so.

  19. locked and limited conversation to collaborators on Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

api-approvedAPI was approved in API review, it can be implementedarea-System.RuntimeblockingMarks issues that we want to fast track in order to unblock other important work

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions