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

Use for..in..do instead of List.iter to prevent function allocations #8175

Merged
merged 1 commit into from
Jan 13, 2020

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Jan 12, 2020

In these specific cases for post inference checks, using List.iter is causing a lot of function allocations. Can be resolved using for..in..do, which is optimized for list meaning that it does not use the enumerator.

Untitled

@forki
Copy link
Contributor

forki commented Jan 12, 2020

What about changing the compiler to rewrite this?

@TIHan
Copy link
Contributor Author

TIHan commented Jan 12, 2020

I want to do that, but it may not be straight forward as we think. It would be quite an interesting optimization. For Array.iter, we inlined the function for that case. I do not want to inline List.iter.

@TIHan
Copy link
Contributor Author

TIHan commented Jan 12, 2020

The easiest thing would be to inline List.iter though. I checked to see if we would need to expose any private internals in order for it to work, but it seems that we might be ok.

I'm generally not comfortable inlining functions in FSharp.Core in order to obtain performance due to potentially exposing private internals that should not be exposed.

@TIHan
Copy link
Contributor Author

TIHan commented Jan 12, 2020

Well, we can't quite inline List.iter - since it requires recursion.

@forki
Copy link
Contributor

forki commented Jan 12, 2020 via email

@TIHan
Copy link
Contributor Author

TIHan commented Jan 12, 2020

It doesn't use a for loop, but I think we could re-implement it using a for loop though.

@TIHan
Copy link
Contributor Author

TIHan commented Jan 12, 2020

I tried inlining List.iter, while it inlined correctly, it didn't stop the function allocation:

internal static void CheckTypesDeep(cenv cenv, FSharpFunc<Tast.TType, Unit> f_0, FSharpOption<FSharpFunc<bool, FSharpFunc<Tast.EntityRef, Unit>>> f_1, FSharpOption<FSharpFunc<Tuple<Tast.EntityRef, FSharpList<Tast.TType>>, Unit>> f_2, FSharpOption<FSharpFunc<Tast.TraitConstraintSln, Unit>> f_3, FSharpOption<FSharpFunc<Tuple<env, Tast.Typar>, Unit>> f_4, TcGlobals.TcGlobals g, env env, FSharpList<Tast.TType> tys)
{
	FSharpFunc<Tast.TType, Unit> fSharpFunc = new CheckTypesDeep@382(cenv, f_0, f_1, f_2, f_3, f_4, g, env);
	FSharpList<Tast.TType> fSharpList = tys;
	for (FSharpList<Tast.TType> tailOrNull = fSharpList.TailOrNull; tailOrNull != null; tailOrNull = fSharpList.TailOrNull)
	{
		Tast.TType headOrDefault = fSharpList.HeadOrDefault;
		fSharpFunc.Invoke(headOrDefault);
		fSharpList = tailOrNull;
	}
}

Why is it still allocating the function? I have no idea. It's probably an optimization bug.

Below is just using this PR:

internal static void CheckTypesDeep(cenv cenv, FSharpFunc<Tast.TType, Unit> f_0, FSharpOption<FSharpFunc<bool, FSharpFunc<Tast.EntityRef, Unit>>> f_1, FSharpOption<FSharpFunc<Tuple<Tast.EntityRef, FSharpList<Tast.TType>>, Unit>> f_2, FSharpOption<FSharpFunc<Tast.TraitConstraintSln, Unit>> f_3, FSharpOption<FSharpFunc<Tuple<env, Tast.Typar>, Unit>> f_4, TcGlobals.TcGlobals g, env env, FSharpList<Tast.TType> tys)
{
	FSharpList<Tast.TType> fSharpList = tys;
	for (FSharpList<Tast.TType> tailOrNull = fSharpList.TailOrNull; tailOrNull != null; tailOrNull = fSharpList.TailOrNull)
	{
		Tast.TType ty = fSharpList.HeadOrDefault;
		CheckTypeDeep(cenv, f_0, f_1, f_2, f_3, f_4, g, env, isInner: true, ty);
		fSharpList = tailOrNull;
	}
}

--

Here are some examples to test out:

open System
open System.Runtime.CompilerServices

let inline iter action (list: 'T list) =
    for x in list do action x
    
//[<MethodImpl(MethodImplOptions.NoInlining)>]
let checkIt (a: int) (b: int) (c: int) (d: int) (e: int) (x: string) = if x = "test" then printf "test"

let check1 a b c d e (x: string list) =
    x |> iter (checkIt a b c d e)
    
let check2 a b c d e (x: string list) =
    for xx in x do checkIt a b c d e xx

check1 will still allocate a function while check2 does not:

    public static void check1(int a, int b, int c, int d, int e, FSharpList<string> x)
    {
        FSharpFunc<string, Unit> fSharpFunc = new check1@11();
        FSharpList<string> fSharpList = x;
        for (FSharpList<string> tailOrNull = fSharpList.TailOrNull; tailOrNull != null; tailOrNull = fSharpList.TailOrNull)
        {
            string headOrDefault = fSharpList.HeadOrDefault;
            fSharpFunc.Invoke(headOrDefault);
            fSharpList = tailOrNull;
        }
    }
    public static void check2(int a, int b, int c, int d, int e, FSharpList<string> x)
    {
        FSharpList<string> fSharpList = x;
        for (FSharpList<string> tailOrNull = fSharpList.TailOrNull; tailOrNull != null; tailOrNull = fSharpList.TailOrNull)
        {
            string headOrDefault = fSharpList.HeadOrDefault;
            if (string.Equals(headOrDefault, "test"))
            {
                PrintfFormat<Unit, TextWriter, Unit, Unit> format = new PrintfFormat<Unit, TextWriter, Unit, Unit, Unit>("test");
                PrintfModule.PrintFormatToTextWriter(Console.Out, format);
            }
            fSharpList = tailOrNull;
        }
    }

--
But, if you get rid of just one parameter, then they are both identical and don't allocate and notice that check2 didn't inline checkIt like before:

let checkIt (a: int) (b: int) (c: int) (d: int) (x: string) = if x = "test" then printf "test"

let check1 a b c d (x: string list) =
    x |> iter (checkIt a b c d)
    
let check2 a b c d (x: string list) =
    for xx in x do checkIt a b c d xx
    public static void check1(int a, int b, int c, int d, FSharpList<string> x)
    {
        FSharpList<string> fSharpList = x;
        for (FSharpList<string> tailOrNull = fSharpList.TailOrNull; tailOrNull != null; tailOrNull = fSharpList.TailOrNull)
        {
            string headOrDefault = fSharpList.HeadOrDefault;
            checkIt(a, b, c, d, headOrDefault);
            fSharpList = tailOrNull;
        }
    }
    public static void check2(int a, int b, int c, int d, FSharpList<string> x)
    {
        FSharpList<string> fSharpList = x;
        for (FSharpList<string> tailOrNull = fSharpList.TailOrNull; tailOrNull != null; tailOrNull = fSharpList.TailOrNull)
        {
            string headOrDefault = fSharpList.HeadOrDefault;
            checkIt(a, b, c, d, headOrDefault);
            fSharpList = tailOrNull;
        }
    }

@TIHan
Copy link
Contributor Author

TIHan commented Jan 12, 2020

In regards to using the inlined version of List.iter and still allocating the function, it has to do with the lambdaInlineThreshold in the optimizer.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

2.5% allocations in a sample is a little too much. Great change!

@cartermp cartermp merged commit e99777c into dotnet:master Jan 13, 2020
@cartermp cartermp mentioned this pull request Jan 13, 2020
10 tasks
@cartermp cartermp mentioned this pull request Jan 17, 2020
5 tasks
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants