RuntimeEnvironmentHelper: remove Lazy<> for basic platform checks#7416
RuntimeEnvironmentHelper: remove Lazy<> for basic platform checks#7416fowl2 wants to merge 1 commit into
Conversation
nkolev92
left a comment
There was a problem hiding this comment.
Let's keep the BSD check on framework too.
| } | ||
| #endif | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Let's just add back the FreeBSD detection.
While mono is technically out of support, I don't want to break anything accidentally.
There was a problem hiding this comment.
Hi @fowl2. Just making sure you saw this comment.
| { | ||
| return System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(System.Runtime.InteropServices.OSPlatform.OSX); | ||
| } | ||
| => System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(System.Runtime.InteropServices.OSPlatform.OSX); |
There was a problem hiding this comment.
It's faster assuming this was called once, but we actually call these methods many many times, for example:
NuGet.Client/src/NuGet.Core/NuGet.Common/PathUtil/PathUtility.cs
Lines 158 to 170 in 1ee6830
I ran the benchmarks:
● Benchmark results (10,000 iterations per call, on Hyper-V):
┌───────────────┬─────────────────┬───────────────────┬─────────────┐
│ Method │ Baseline (Lazy) │ Proposed (direct) │ Δ │
├───────────────┼─────────────────┼───────────────────┼─────────────┤
│ IsWindows │ 8.59 µs │ 6.74 µs │ -22% ✅ │
├───────────────┼─────────────────┼───────────────────┼─────────────┤
│ IsMacOSX │ 6.61 µs │ 8.14 µs │ +23% ❌ │
├───────────────┼─────────────────┼───────────────────┼─────────────┤
│ IsLinux │ 6.56 µs │ 3.93 µs │ -40% ✅ │
├───────────────┼─────────────────┼───────────────────┼─────────────┤
│ IsMono │ 11.53 µs │ 3.91 µs │ -66% ✅ │
There was a problem hiding this comment.
I didn't really add a summary here :D
This is me to say the change is good, but I think it would've been great to see this type of benchmark in the PR body to begin with.
|
This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 30 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch. |
IsOSPlatform()is fast, so remove all the overhead from using a bunch ofLazy<>.IsLinuxalso returnstrueon BSD, not sure if that's documented, but switched to conditional compilation to remove an allocation.I'm not sure if Mono is still supported - if it's not, ripping all that code out might be a good follow up.