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

Intrinsify RuntimeInformation.IsOSPlatform #738

Closed
am11 opened this issue Dec 10, 2019 · 12 comments · Fixed by #83308
Closed

Intrinsify RuntimeInformation.IsOSPlatform #738

am11 opened this issue Dec 10, 2019 · 12 comments · Fixed by #83308

Comments

@am11
Copy link
Member

am11 commented Dec 10, 2019

The result of RuntimeInformation.IsOSPlatform does not change during the application run lifecycle. It would be a nice optimization if JIT intrinsically generate a better code for this method.

For example, for the following method:

private static bool CheckIfOSIsWindows() =>
  RuntimeInformation.IsOSPlatform(OSPlatform.Windows);

JIT produces this asm (with VS2019 and .NET Core 3.1 in Release configuration):

           RuntimeInformation.IsOSPlatform(OSPlatform.Windows);
00007FF810DC0FB2  sub         esp,30h  
00007FF810DC0FB5  lea         rbp,[rsp+30h]  
00007FF810DC0FBA  xor         eax,eax  
00007FF810DC0FBC  mov         qword ptr [rbp-8],rax  
00007FF810DC0FC0  call        00007FF810DC0670  
00007FF810DC0FC5  mov         qword ptr [rbp-8],rax  
00007FF810DC0FC9  mov         rcx,qword ptr [rbp-8]  
00007FF810DC0FCD  call        00007FF810DC0778  
00007FF810DC0FD2  nop  
00007FF810DC0FD3  lea         rsp,[rbp]  
00007FF810DC0FD7  pop         rbp  
00007FF810DC0FD8  ret  

Instead, it could just produce:

xor     eax, 1
ret

on Windows, and on Unix:

xor     eax, eax
ret
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Dec 10, 2019
@danmoseley
Copy link
Member

Presumably that could potentially enable the JIT to remove the code as dead.

How common is it to check this though, and is it normally a perf concern?

@am11
Copy link
Member Author

am11 commented Dec 10, 2019

I think it belongs to the same category as branch elimination due to Avx.IsSupported etc. (today, which is not there yet as well: https://github.com/dotnet/coreclr/issues/27935). While profiling some code today, I found some noise cause by RuntimeInformation branches.

@jkotas
Copy link
Member

jkotas commented Dec 10, 2019

branch elimination due to Avx.IsSupported

These APIs were specifically designed to be friendly to JIT optimizations

RuntimeInformation.IsOSPlatform

This API is not friendly to JIT optimizations. It can be optimized quite a bit by having a better implementation, without any help from the JIT. These optimizations should be done first.

@huoyaoyuan
Copy link
Member

How common is it to check this though, and is it normally a perf concern?

It can reduce code size in common scenarios. Platform specific code may be large when calling a sequence of P/Invoke.

@jeffschwMSFT jeffschwMSFT added this to the Future milestone Dec 11, 2019
@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Dec 11, 2019
@tannergooding
Copy link
Member

It can be optimized quite a bit by having a better implementation, without any help from the JIT. These optimizations should be done first.

@jkotas, do you have a suggestion on how to do this?

The implementation today is trivial and just compares the input struct with the static readonly instance member for the current platform:

public static bool IsOSPlatform(OSPlatform osPlatform)
{
return OSPlatform.Windows == osPlatform;
}

Which ultimately just compares the internal string member using string.Equals(_osPlatform, other, StringComparison.Ordinal);.

I can't think of any existing JIT optimizations that would allow us to improve this so that it could be constant folded for the common case of IsOSPlatform(OSPlatform.Windows), etc.

@tannergooding
Copy link
Member

Looks like Unix has a slightly different implementation, but I think the same question applies:

public static bool IsOSPlatform(OSPlatform osPlatform)
{
string name = s_osPlatformName ??= Interop.Sys.GetUnixName();
return osPlatform.Equals(name);
}
(thanks for pointing that out @eerhardt)

@eerhardt
Copy link
Member

Another case where this would help is in AOT situations. If I was compiling for linux-x64, and if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) was turned into if (false), then whole chunks of code wouldn't need to be compiled and could be eliminated in the final assembly. For example:

                    if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
                    {
                        // bring in a whole new dependency that is Windows-specific
                    }

It could eliminate a whole dependency completely in a linux AOT/crossgen'd version.

Note that this is the pattern we recommend to developers to use for switching on Operating Systems. There currently isn't a built-in way to pivot your C# compilation based on target OS.

@jkotas
Copy link
Member

jkotas commented Feb 12, 2020

There is an effort to do this in the linker: dotnet/linker#607 . Doing this in the linker will strip it from both IL and potential AOT image. Stripping it from both is better than stripping it from just AOT image.

@jkotas
Copy link
Member

jkotas commented Feb 13, 2020

I can't think of any existing JIT optimizations that would allow us to improve this

For example, we can normalize the string into int in the struct constructor (keeping a global table with all OS strings) and then the equals method will become a comparison of two ints. The JIT may need fixes to do the constant prop for these two ints, but that would be a reasonable general-purpose optimization to have.

This approach has trade-offs so it is not obvious that we should do it. I am just trying to demonstrate that there are options.

// bring in a whole new dependency that is Windows-specific

To avoid bringing in the whole new dependency, the code should better be moved into a separate method. Even if we intrinsify this in the JIT, the JIT would still bring in the inline dependency in in the common case before optimizing it out.

Note that this is the pattern we recommend to developers to use for switching on Operating Systems.

Do we have it written anywhere? This pattern is a non-starter in many cases given the current performance characteristics of this API. RuntimeInformation is a slow type. One has to cache the results locally in a readonly static if it is used for anything on a frequently executed path.

@reflectronic
Copy link
Contributor

Do we have it written anywhere?

Yes:

https://github.com/dotnet/platform-compat/blob/master/docs/DE0007.md

DE0007: Platform ID shouldn't be used

...
Code that compares Environment.OSVersion.Platform to PlatformID.MacOSX
should use the newer RuntimeInformation.IsOSPlatform(OSPlatform.OSX) method instead.

https://github.com/dotnet/platform-compat/blob/master/docs/DE0009.md

DE0009: Environment.OSVersion shouldn't be used

...
Use the RuntimeInformation.IsOSPlatform method to identify the OS platform. Avoid writing code dependent on reported OS version and instead check for availability of the needed features.

MichalStrehovsky pushed a commit to MichalStrehovsky/runtime that referenced this issue Mar 25, 2021
Also enable cross-building and publishing Windows ARM64-hosted objwriter.dll. Use Release build type with debug information enabled.
@jkoritzinsky
Copy link
Member

The new methods on the System.OperatingSystem class are simple enough to be JIT-time constants for optimized code. They're also recommended as the best way to check the current OS platform.

I'll close this issue and recommended to use the method on the OperatingSystem class to detect the OS in a way the JIT optimizes well.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
@EgorBo
Copy link
Member

EgorBo commented Jul 1, 2023

NOTE: it's handled in .NET 8.0 now in JIT as well (it wasn't intrisified specially, but a set of unrelated general optimizations helped to make this happen)

class Tests
{
    public static bool IsWin() => RuntimeInformation.IsOSPlatform(OSPlatform.Windows);
    public static bool IsLinux() => RuntimeInformation.IsOSPlatform(OSPlatform.Linux);
}

Codegen (e.g. on Windows):

; Assembly listing for method Tests:IsWin():bool (Tier1)
       mov      eax, 1
       ret 

; Assembly listing for method Tests:IsLinux():bool (Tier1)
       xor      eax, eax
       ret 

Only for JIT and only if promoted to Tier1 naturally so sharplab won't show it.

NativeAOT is blocked by #83043

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.