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.

Sign up for GitHub

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

Jump to bottom

Use Environment.WorkingSet to improve performance of Process.Metrics #2286

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Use Environment.WorkingSet to improve performance of Process.Metrics #2286

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
97edb4a
Cache Process object to avoid performance hit
danespinosa Nov 1, 2024
7f8bc63
Revert "Cache Process object to avoid performance hit"
Nov 6, 2024
d05a377
read working set from Environment since it has better perf
Nov 7, 2024
a1b0806
Merge branch 'main' into main
Kielek Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions src/OpenTelemetry.Instrumentation.Process/ProcessMetrics.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,15 @@ internal sealed class ProcessMetrics

private static readonly Meter MeterInstance = new(MeterName, Assembly.GetPackageVersion());

private static readonly Diagnostics.Process Process = Diagnostics.Process.GetCurrentProcess();

static ProcessMetrics()
{
MeterInstance.CreateObservableUpDownCounter(
"process.memory.usage",
() =>
{
using var process = Diagnostics.Process.GetCurrentProcess();
return process.WorkingSet64;
return Process.WorkingSet64;
Copy link
Contributor

@haipz haipz Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Process.WorkingSet64 won't change unless call Process.Refresh() before accessing the property(WorkingSet64).

Copy link
Author

Choose a reason for hiding this comment

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

You are right @haipz probably the class will need some redesign, e.g. maybe the ProcessInstrumentationOptions could accept a refresh rate at which we could refresh the Process. I will try to get ahold of @Yun-Ting to discuss this.

Copy link
Author

Choose a reason for hiding this comment

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

An option to this is using Environment.WorkingSet which has no perf implications and displays the same data with minimal change
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @danespinosa, I've gone down the cache path before. Check out the discussions here: #718.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for sharing @Yun-Ting i see that your implementation got updated intending to solve an issue but it really didn't solve the issue perse. Here is the commit [Instrumentation.Process, Resources.Process] Properly disposes of Sys… · open-telemetry/opentelemetry-dotnet-contrib@55c0dfb and here is the issue that explains at the bottom that actually the issue persists Properly dispose instances of System.Diagnostics.Process class · Issue #2100 · open-telemetry/opentelemetry-dotnet-contrib.

My 2 suggestions and 2 cents, could we revert that PR and bring back your implementation or can we at least update the process.WorkingSet64 call to instead calling Environment.WorkingSet? This will bring huge gains
image

Copy link
Author

Choose a reason for hiding this comment

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

@noahfalk I was chatting with @Yun-Ting regarding the perf or this library and we were wondering if we could get your thoughts about this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a fine change to me.

Looking at #718 I see that it relied on some benchmarking in #717. The benchmarking tested various ways of running the Refresh() or GetCurrentProcess() in isolation and then #718 extrapolated that if the calls were fast in isolation then it would also be fast in the process metrics scenarios where we invoke various property getters on the Process object afterwards. Unforetunately that isn't the case. The property getters populate data lazily if it isn't already populated and that winds up being the expensive part of the operation. Calling GetCurrentProcess() returns a process object where the values haven't been initialized and Refresh() only invalidates the cached data but doesn't fetch new data. Calling GetCurrentProcess() or Refresh() repeatedly costs very little. Also calling WorkingSet on an initialized Process object costs very little. But the first call to WorkingSet after Refresh() or GetCurrentProcess() is comparatively very expensive:

| RefreshOnly                    |         2.017 ns |      0.0305 ns |       0.0270 ns |
| GetCurrentProcessOnly          |        47.814 ns |      0.9146 ns |       1.1567 ns |
| WorkingSetOnly                 |         1.110 ns |      0.0231 ns |       0.0216 ns |
| RefreshAndWorkingSet           | 3,241,750.985 ns | 58,170.7094 ns | 109,258.7011 ns |
| GetCurrentProcessAndWorkingSet | 3,243,749.014 ns | 63,792.7044 ns |  75,940.6834 ns |

Benchmark code:

    public class ProcessBenchmarks
    {
        private static Process s_process = Process.GetCurrentProcess();


        [Benchmark]
        public void RefreshOnly()
        {
            s_process.Refresh();
        }

        [Benchmark]
        public Process GetCurrentProcessOnly()
        {
            return Process.GetCurrentProcess();
        }

        [Benchmark]
        public long WorkingSetOnly()
        {
            return s_process.WorkingSet64;
        }

        [Benchmark]
        public long RefreshAndWorkingSet()
        {
            s_process.Refresh();
            return s_process.WorkingSet64;
        }

        [Benchmark]
        public long GetCurrentProcessAndWorkingSet()
        {
            return Process.GetCurrentProcess().WorkingSet64;
        }
    }

In the .NET source here this is the cache check. If processInfo == null the property getter will take milliseconds, if its already cached the getter will be nanoseconds. Hope that helps!

Copy link

Choose a reason for hiding this comment

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

This change all good to merge then?

Copy link
Author

Choose a reason for hiding this comment

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

My apologies, I have been busy with other things and this took second priority, I'll be working on not only updating the method to get the WorkingSet but also the Memory VirtualMemorySize64. Then in other PRs i'll be improving CPU and ThreadCount and by bringing the logic from the .NET metrics hopefully it's not too hard :)

},
unit: "By",
description: "The amount of physical memory in use.");
Expand All @@ -34,8 +35,7 @@ static ProcessMetrics()
"process.memory.virtual",
() =>
{
using var process = Diagnostics.Process.GetCurrentProcess();
return process.VirtualMemorySize64;
return Process.VirtualMemorySize64;
},
unit: "By",
description: "The amount of committed virtual memory.");
Expand All @@ -44,11 +44,10 @@ static ProcessMetrics()
"process.cpu.time",
() =>
{
using var process = Diagnostics.Process.GetCurrentProcess();
return new[]
{
new Measurement<double>(process.UserProcessorTime.TotalSeconds, new KeyValuePair<string, object?>("process.cpu.state", "user")),
new Measurement<double>(process.PrivilegedProcessorTime.TotalSeconds, new KeyValuePair<string, object?>("process.cpu.state", "system")),
new Measurement<double>(Process.UserProcessorTime.TotalSeconds, new KeyValuePair<string, object?>("process.cpu.state", "user")),
new Measurement<double>(Process.PrivilegedProcessorTime.TotalSeconds, new KeyValuePair<string, object?>("process.cpu.state", "system")),
};
},
unit: "s",
Expand All @@ -67,8 +66,7 @@ static ProcessMetrics()
"process.thread.count",
() =>
{
using var process = Diagnostics.Process.GetCurrentProcess();
return process.Threads.Count;
return Process.Threads.Count;
},
unit: "{thread}",
description: "Process threads count.");
Expand Down