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

Compilation gets much slower when using many computation expressions #14429

Open
alfonsogarciacaro opened this issue Dec 3, 2022 · 23 comments
Open
Labels
Area-ComputationExpressions End-to-end experience for computation expressions (except async and state machine compilation) Theme-Performance
Milestone

Comments

@alfonsogarciacaro
Copy link
Contributor

In a big Fable project, after a big refactoring to introduce a convenience computation expression, the project started to take much longer to compile. The overhead came from the type checking phase of FSharp.Compiler.Service (it also takes a long time when running dotnet build). The computation expression is a convenience to generate React fragments out of React elements. The CE accepts single React Elements as well as sequences and optional elements. The code is like this:

type ElementBuilder() =
    member _.Zero () : ReactElements =
        []

    member _.Yield (e: ReactElement) : ReactElements =
        [e]

    member _.Yield (maybeE: Option<ReactElement>) : ReactElements =
        match maybeE with
        | None   -> []
        | Some e -> [e]

    member _.Yield (maybeEs: Option<List<ReactElement>>) : ReactElements =
        match maybeEs with
        | None    -> []
        | Some es -> es

    member _.Yield (es: List<ReactElement>) : ReactElements =
        es

    member _.Yield (es: seq<ReactElement>) : ReactElements =
        Seq.toList es

    member _.Yield (ess: list<list<ReactElement>>) : ReactElements =
        List.flatten ess

    member _.Yield (ess: seq<list<ReactElement>>) : ReactElements =
        ess |> Seq.collect id |> Seq.toList

    member _.Combine (e: ReactElement, es: ReactElements) : ReactElements =
        e :: es

    member _.Combine (moreEs: List<ReactElement>, es: ReactElements) : ReactElements =
        moreEs @ es

    member _.Combine (moreEs: seq<ReactElement>, es: ReactElements) : ReactElements =
        (Seq.toList moreEs) @ es

    member _.Combine (moreEs: List<List<ReactElement>>, f: unit -> ReactElements) : ReactElements =
        (List.flatten moreEs) @ (f())

    member _.Combine (e: ReactElement, f: unit -> ReactElements) : ReactElements =
        e :: (f())

    member _.Combine (moreEs: List<ReactElement>, f: unit -> ReactElements) : ReactElements =
        moreEs @ (f())

    member _.Combine (moreEs: seq<ReactElement>, f: unit -> ReactElements) : ReactElements =
        (Seq.toList moreEs) @ (f())

    member _.Combine (moreEs: List<List<ReactElement>>, es: ReactElements) : ReactElements =
        (List.flatten moreEs) @ es

    member _.Delay (expr: unit -> ReactElements) : unit -> ReactElements = expr

    member _.Run (f: unit -> ReactElements) : ReactElement =
        let elements = f()
        Fable.React.Helpers.fragment [] elements

let element = ElementBuilder()

There's a similar CE ElementsBuilder that works the same but returns ReactElement list instead of the fragment.

Users tried changing the CE in different ways, but the only thing that worked was to remove the CE in most of the files. It is still used in some of them but compilation times are mostly back to normal.

The most interesting part is I tried profiling this, and the result is most of the time is spent in the function FSharp.Compiler.NameResolution.ResolveLongIdentAsModuleOrNamespaceThen. This makes me wonder if the CE is not responsible for the slow compilation but instead it causes a problematic name resolution directly. Both ElementBuilder and element are in the same module decorated with AutoOpen.

image

Any hints to improve the performance of the compilation?

@github-actions github-actions bot added this to the Backlog milestone Dec 3, 2022
@KevinRansom
Copy link
Member

I think this is a pretty decent repro:

ConsoleApp20.zip

The response files are set up for my laptop.
But build it /v:normal, make 2 response files
1 ConsoleApp20.Program.ce.rsp
add this: --times
add this: --define:PROGRAM_CE

  1. ConsoleApp20.Program.rsp
    add this: --times
    add this: --define:PROGRAM

run the first (non-ce) using:

fsc @ConsoleApp20.Program.rsp

Observe:

Real:  0.8 Realdelta:  0.1 Cpu:  0.5 Cpudelta:  0.0 Mem: 145 G0:   1 G1:  1 G2:  0 [Write .NET Binary]

Repeat for ce response:

 fsc @ConsoleApp20.Program.ce.rsp

observe:

Real:  2.6 Realdelta:  0.4 Cpu:  0.8 Cpudelta:  0.2 Mem: 182 G0:   2 G1:  1 G2:  1 [Write .NET Binary]

from .8 up to 2.6 seconds with a constant about a half second start up penalty in each case would tend to indicate, some super expensive operations happening in the CE case.

@En3Tho
Copy link
Contributor

En3Tho commented Dec 5, 2022

In my case what really slowed down highlighting and checking was adding more cases to yield-combine combo.

Idea is simple: if you yield some items then you can yield only those, I'd you yield other then only other.

So before there was only one "type-path" in my CE and it was super fast. After I've implemented a new yield-combine-... to make an additional "type-path" intellisense and highlighting started to show considerable amount of lag

By "type-path" I mean resulting type in CE operations, in this example it's ReactElements. In my case I have roughly speaking ReactElements and ReactElements2.

@0101 0101 added Theme-Performance Area-ComputationExpressions End-to-end experience for computation expressions (except async and state machine compilation) and removed Needs-Triage labels Mar 20, 2023
@T-Gro T-Gro self-assigned this Mar 20, 2023
@albertwoo
Copy link

Similar issue hanppens to my library too: #12723

@T-Gro
Copy link
Member

T-Gro commented Apr 13, 2023

Yes, this is the same phenomena.

I want to have a look at this eventually to see if the resolution approach could be tuned while maintaining the same behavior, but I can already see this is a risky path.

If I fail to improve it in general (very possible), I want to at least come up with a recommendation to keep things fast.
Not surprisingly, the nicer you want the final DSL to be (supporting more types via DSLs, nesting of CEs), the slower the compilation gets.

@En3Tho
Copy link
Contributor

En3Tho commented Apr 13, 2023

@albertwoo It's funny that your CE in question is blazor-based. So is mine :) I had to introduce different types/overloads to split css and elements (e.g. if you do div { style... <- only css is allowed here } { ... html is allowed again } or div { div ... only html is allowed }. That made perf go bonkers

In your case I see from that issue is that you don't do this kind of split but perf is still not okay?

@albertwoo
Copy link

@En3Tho yes it is for blazor. Here is my repo: https://github.com/slaveOftime/Fun.Blazor
Normally it looks better to write like:

div {
    class' "..."
    onclick (fun _ -> ())
    h1 { "foo" }
    p { "bar" }
}

But after I found the performance issue, I introduced a CustomOperation "childContent", which can improve the performance a lot:

div {
    class' "..."
    onclick (fun _ -> ())
    childContent [
        h1 { "foo" }
        p { "bar" }
    ]
}

But still, CE DSL is slow for compile and autocomplete when project grows. There are definitely some places to improve, for example, one character change in a string can make the whole solution (multiple projects) rebuild which is so annoying, expecially there is not hot-reload supported in fsharp.

@albertwoo
Copy link

I also found that the number of CustomOperations which ared defined in the CE builder will impact the compilation speed a lot. Maybe fsharp compilier can add some cache to improve the lookup for CustomOperations.

@albertwoo
Copy link

There is another thing which may also be related, for example, in below picture, the CutsomOperations from button CE will display in nested style CE or functions (onclick is button's CustomOperations) which is not expected right? Becuase it listed in the auto completion list but after you use it there is an error popup:

image

image

This issue will cause situation: the deeper you nest your CE, the slower you get your intellisense and compilation. Because it will spend more time to lookup other CE's CustomOperations.

@patrickallensimpson
Copy link

What's the status on this, has this been identified yet?

@vzarytovskii
Copy link
Member

What's the status on this, has this been identified yet?

I started looking at it but got sidetracked. Going to continue once finish my nullables work

@patrickallensimpson
Copy link

patrickallensimpson commented Jul 13, 2023

This is becoming a show stopper for us, we are using Fun.Blazor for our UI work. And I'm seeing 50% CPU utilization on a iProcessor 13th Gen Intel(R) Core(TM) i9-13900K, 3000 Mhz, 24 Core(s), 32 Logical Processor(s)
. Do you have any idea where this is happening because I'll attempt to work on this.

I just started to build the tooling from source for other reasons.

@vzarytovskii
Copy link
Member

This is becoming a show stopper for us, we are using Fun.Blazor for our UI work. And I'm seeing 50% CPU utilization on a iProcessor 13th Gen Intel(R) Core(TM) i9-13900K, 3000 Mhz, 24 Core(s), 32 Logical Processor(s)
. Do you have any idea where this is happening because I'll attempt to work on this.

I just started to build the tooling from source for other reasons.

Yeah, it's pretty much we do tc/resolutions for custom operations all the time from scratch.
Certain things can be cached there.

I know @T-Gro was looking into some ce-heavy code too.

If you have example project, or can synthesize it, I can show how it can be profiled with ease.

@edgarfgp
Copy link
Contributor

edgarfgp commented Oct 16, 2023

It would be really helpful to get some improvements here. CE's are really "used|abused" in F# and the compilation goes really slow at some points .

@patrickallensimpson
Copy link

Was there an update to this issue that would resolve this? I hadn't seen anything in the Release notes that would indicate this?

Thanks,
-Patrick

@vzarytovskii
Copy link
Member

Was there an update to this issue that would resolve this? I hadn't seen anything in the Release notes that would indicate this?

Thanks,

-Patrick

Not to my knowledge, no changes to CEs checking.

@albertwoo
Copy link

@vzarytovskii @T-Gro any progress on this, it's been a while since last update.

We have multiple projects are impacted by this issue.

Please please improve this!!! Many many thanks!!!

@edgarfgp
Copy link
Contributor

edgarfgp commented Jan 9, 2024

Nested CE + large DU pattern matching is getting really slow.

@vzarytovskii
Copy link
Member

@vzarytovskii @T-Gro any progress on this, it's been a while since last update.

We have multiple projects are impacted by this issue.

Please please improve this!!! Many many thanks!!!

No progress really, I suspect it will require substantial changes and investigation.

It is committed for F#9, but I don't have a specific timeline.

@vzarytovskii
Copy link
Member

I had multiple attempts to rewrite how caching is done for the CEs, but with no luck.

@nojaf
Copy link
Contributor

nojaf commented Jan 10, 2024

Were you able to record any of your findings somewhere?
This is on my list to investigate as well, it would be great if there were prior attempts to learn from.

@vzarytovskii
Copy link
Member

Were you able to record any of your findings somewhere? This is on my list to investigate as well, it would be great if there were prior attempts to learn from.

No writings per se, but I would be happy to meet and explain the problem. Or rather one of the problems.

@albertwoo
Copy link

@vzarytovskii can we introduce some attributes or something else which the library author can use it to annotate the CE based DSL so the compiler can use it to compile code faster without the library consumer to do anything?

If this can help, I think it is also acceptable and maybe a direction to give a try, especially if there is no way out for solving this performance issue because of how the CE currently works.

@vzarytovskii
Copy link
Member

@vzarytovskii can we introduce some attributes or something else which the library author can use it to annotate the CE based DSL so the compiler can use it to compile code faster without the library consumer to do anything?

If this can help, I think it is also acceptable and maybe a direction to give a try, especially if there is no way out for solving this performance issue because of how the CE currently works.

Unlikely, CE typechecking needs a significant rewrite for it to improve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-ComputationExpressions End-to-end experience for computation expressions (except async and state machine compilation) Theme-Performance
Projects
Status: New
Development

No branches or pull requests

10 participants