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

Avoid creating external processes to read OS version info #1240

Merged
merged 2 commits into from
May 15, 2023

Conversation

mjcheetham
Copy link
Collaborator

@mjcheetham mjcheetham commented May 3, 2023

We are spending about 45ms on macOS to read version information by shelling out to sw_vers on each run of GCM.

Let's try and be smarter and use BCL APIs, system APIs or file-based sources for the OS and distribution information before resorting to external processes.

In my testing, on macOS, we went from 45ms to 4.5ms (10x speedup).

Note that we are now also getting something a little bit more useful for Linux:

Before:

$ GCM_TRACE=1 ./git-credential-manager --version
20:55:36.776385 ...re/Application.cs:95 trace: [RunInternalAsync] Version: 2.1.1.0
20:55:36.787669 ...re/Application.cs:96 trace: [RunInternalAsync] Runtime: .NET 6.0.16
20:55:36.787711 ...re/Application.cs:97 trace: [RunInternalAsync] Platform: Linux (x86-64)
20:55:36.787749 ...re/Application.cs:98 trace: [RunInternalAsync] OSVersion: Linux ubuntu2204-vm1 5.15.0-1037-azure #44-Ubuntu SMP Thu Apr 20 13:19:31 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
..

After:

$ GCM_TRACE=1 ./git-credential-manager --version
20:52:12.325628 ...re/Application.cs:95 trace: [RunInternalAsync] Version: 2.1.1.0
20:52:12.337234 ...re/Application.cs:96 trace: [RunInternalAsync] Runtime: .NET 6.0.16
20:52:12.337283 ...re/Application.cs:97 trace: [RunInternalAsync] Platform: Linux (x86-64)
20:52:12.337301 ...re/Application.cs:98 trace: [RunInternalAsync] OSVersion: Ubuntu 22.04.1 LTS
..

@mjcheetham mjcheetham added the performance An issue with performance label May 3, 2023
Since .NET 5 we can now use the `Environment.OSVersion` property to
lookup the real OS version for macOS and Windows[1].

For .NET Framework (still used for our releases on Windows) we must
continue to use the Win32 API `RtlGetVersion` to avoid any Windows
compatibility nonsense.

We continue to use `uname` on Linux.

[1] https://learn.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/5.0/environment-osversion-returns-correct-version
@mjcheetham mjcheetham marked this pull request as ready for review May 3, 2023 23:09
@mjcheetham mjcheetham force-pushed the perf2 branch 2 times, most recently from a5e3f4a to b7aeb3a Compare May 4, 2023 15:39
Copy link
Contributor

@ldennington ldennington left a comment

Choose a reason for hiding this comment

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

Looks good!

return swvers.StandardOutput.ReadToEnd().Trim();
}
}
return Environment.OSVersion.VersionString;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Try using the /etc/os-release file, for systemd distros, in favour of
calling out to `uname` which can add extra overhead in the form of
process startup. Direct file I/O and parsing should be faster.
@mjcheetham mjcheetham merged commit 1110086 into git-ecosystem:main May 15, 2023
@mjcheetham mjcheetham deleted the perf2 branch May 15, 2023 20:52
@mjcheetham mjcheetham mentioned this pull request Jun 26, 2023
mjcheetham added a commit that referenced this pull request Jun 26, 2023
**Changes:**

- Use in-proc methods for getting OS version number (#1240, #1264)
- Update System.CommandLine (#1265)
- Suppress GUI from command-line argument (#1267)
- Add github (login|logout|list) commands (#1267)
- cURL Cookie file support (#1251)
- Update target framework on Mac/Linux to .NET 7 (#1274, #1282)
- Replace JSON.NET with System.Text.Json (#1274)
- Preserve exact redirect URI formatting in OAuth requests (#1281)
- Use IP localhost redirect for GitHub (#1286)
- Use WWW-Authenticate headers from Git for Azure Repos authority
(#1288)
- Better GitHub Enterprise Managed User (EMU) account support (#1190)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance An issue with performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants