Use Environment.WorkingSet to improve performance of Process.Metrics

[danespinosa]

Fixes #
Design discussion issue #

Based on traces GetProcessInfos which is called by DiagnosticsGetProcess has a big perf hit. Proposing using Environment.WorkingSet to improve the performance of this API.

Changes

Please provide a brief description of the changes here.
This PR replaces the usage of a new Process to get its working set in favor of using Environment.WorkingSet instead. See benchmark below.

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[linux-foundation-easycla]

• ✅ login: danespinosa / name: Dan Espinosa (97edb4a)
• ✅ login: Kielek / name: Piotr Kiełkowicz (a1b0806)
• ❌ The email address for the commit (d05a377, 7f8bc63) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

[haipz]

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

[danespinosa]

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.

[danespinosa]

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

[Yun-Ting]

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

[danespinosa]

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

[danespinosa]

@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.

[noahfalk]

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!

[davidfowl]

This change all good to merge then?

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.