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

Implement __assume for non-windows builds #8544

Open
AndyAyersMS opened this issue Jul 14, 2017 · 3 comments
Open

Implement __assume for non-windows builds #8544

AndyAyersMS opened this issue Jul 14, 2017 · 3 comments
Labels
area-PAL-coreclr enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors os-freebsd FreeBSD OS os-linux Linux OS (any supported distro) os-mac-os-x macOS aka OSX tenet-performance Performance related issue
Milestone

Comments

@AndyAyersMS
Copy link
Member

MethodTable::GetManagedClassObjectIfExists() uses COMPILER_ASSUME to help the native compiler avoid a null check and branch in the common case.

COMPILER_ASSUME expands to __assume but the latter is defined in pal.h as (void)0 when not compiling with MSVC. Seems like we ought to be able try clang's __builtin_assume here and get similar benefits.

MethodTable::GetManagedClassObjectIfExists() is fairly hot since it is called in the fast path of Object.GetType().

If this works in Clang as it does in MSVC, it should give a small improvement in the performance of Object.GetType(), as in the attached program.

using System;

public class GetType
{
    public static volatile anObject = null;

    public static Type TypeReflectionObjectGetType(long count)
    {
        Type type = null;
        anObject = "aString";
        
        for (long i = 0; i < count; i++)
        {
            type = anObject.GetType();
        }
        
        return type;
    }
    
    public static int Main()
    {
        long count = 100000;
        Stopwatch sw = Stopwatch.StartNew();
        Type t = TypeReflectionObjectGetType(count);
        sw.Stop();
        Console.WriteLine("GetType: {0} iterations in {1:F2} ms", count, sw.Elapsed.TotalMilliseconds);
        return (t == typeof(string) ? 100 : 0);
    }
}
@wecing
Copy link

wecing commented Sep 12, 2017

(cc @jkotas) It seems that clang warns about side effects in __builtin_assume by default, but VS (C4557) doesn't. Because of -Werror, implementing this feature would produce errors for perfectly fine use cases like this:

__assume(GetNativeHeader() != NULL);
return GetNativeHeader()->ImageBase;

Then should we manually rewrite all use cases of __assume to:

CORCOMPILE_HEADER *h = GetNativeHeader();
__assume(h != NULL);
return h->ImageBase;

, or use -Wno-assume globally?

@jkotas
Copy link
Member

jkotas commented Sep 12, 2017

I think it should be fine to use -Wno-assume globally.

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@ghost
Copy link

ghost commented Feb 22, 2024

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of our issue cleanup automation.

@ghost ghost added backlog-cleanup-candidate An inactive issue that has been marked for automated closure. no-recent-activity labels Feb 22, 2024
@jkotas jkotas removed no-recent-activity backlog-cleanup-candidate An inactive issue that has been marked for automated closure. labels Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-PAL-coreclr enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors os-freebsd FreeBSD OS os-linux Linux OS (any supported distro) os-mac-os-x macOS aka OSX tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

4 participants