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

Performance decrease when running FSharpChecker.ParseAndCheckProject #12380

Closed
alfonsogarciacaro opened this issue Nov 12, 2021 · 12 comments
Closed

Comments

@alfonsogarciacaro
Copy link
Contributor

I'm trying to update Fable to FCS 41.0.1. It seemed to work fine, however when I run it against a very big project I noticed ParseAndCheckProject takes much longer now (call site in Fable). This project includes 1275 files (note that Fable needs to collect all sources including references and packages to get the expression trees). After trying with previous FSharp.Compiler.Service Nuget packages I realized the jump happened between 40.0 and 40.0.1-preview.21352.5, these are the times for the first ParseAndCheckProject call:

v40.0:                   79000ms
v40.0.1-preview.21352.5: 124918ms

The difference doesn't seem to be so big in medium-sized projects. Do you have an idea which change 40.0.1-preview may have caused performance to decrease faster when there are many files?

As an aside, the change logs for each of the published packages are a bit messy now. I couldn't find a change log for 40.0.1-preview.21352.5 and the release notes for 41.0 only point to this issue, so it's difficult to know what has changed in the FCS package, and which commit triggered each release.

@dsyme
Copy link
Contributor

dsyme commented Nov 12, 2021

Hmmmmm.... Thanks for the report. Can you get any profiling data for the two? Also if we have SHA for the two we can compare.

@dsyme dsyme changed the title Drastic performance decrease when running FSharpChecker.ParseAndCheckProject Performance decrease when running FSharpChecker.ParseAndCheckProject Nov 12, 2021
@dsyme
Copy link
Contributor

dsyme commented Nov 12, 2021

Do you have an idea which change 40.0.1-preview may have caused performance to decrease faster when there are many files?

Not specifically. You might try to isolate phases (try --parseonly for example to isolate the parsing time), or to binary chop your files to see if it's generic across all files or related to a few specific files that use one construct (computation expressions, or SRTP, or method overloading, or ...)

@inosik
Copy link

inosik commented Nov 12, 2021

Also if we have SHA for the two we can compare.

v40.0.0 was built from 4aa714b7, v40.0.1-preview.21352.5 from 3af67cd8. Fuget has a link to the source repository and the respective commit.

Diff: 4aa714b...3af67cd.

@dsyme
Copy link
Contributor

dsyme commented Nov 12, 2021

I'd like to get to the bottom of this for sure. Note that we are doing some extra checking in F# 6 (some type constraints in that were not being checked properly), so I wouldn't automatically consider this a bug if it's that kind of thing, though we would need to confirm that and take a good look if the extra costs are needed

Alternatively it could be some other problem. Let's find out somehow. A binary chop on PRs or inputs anything to isolate the problem

@alfonsogarciacaro
Copy link
Contributor Author

Thanks for that @inosik! So the commit for v40.0.1-preview is from 2021-07-03. Actually I went back to a custom FCS build. I rebased dotnet/fsharp from 2021-06-08 because I needed this fix. The build is working fine and there's less than a month of changes to v40.0.1-preview so it should be easier to isolate the problem.

Thanks for checking this @dsyme! I will try to isolate and profile. Though I need to fix Visual Studio before :) VS 2022 preview was working great but now I installed VS 2022 Community and I cannot open most of F# projects, maybe I missed some needed components during the custom installation.

@alfonsogarciacaro
Copy link
Contributor Author

UPDATE: I tried syncing ncave's service_slim fork with latest dotnet/fsharp and the performance is good (a bit slower than before that seems in line with Don Syme's comments that the compiler is doing more work now). service_slim bypasses IncrementalBuild and calls TypeCheckClosedInputSet directly, so I assume the bottle neck must be in IncrementalBuild.fs.

@dsyme
Copy link
Contributor

dsyme commented Nov 18, 2021

I assume the bottle neck must be in IncrementalBuild.fs.

I think we'd need a profiling run to action this. @TIHan I don't know of any changes in IncrementalBuild.fs that would have caused such a slow down?

@alfonsogarciacaro
Copy link
Contributor Author

Here's the profiling data I got with VS 2019 compiling the big project with v40.0.1-preview. I hope it helps:

https://1drv.ms/u/s!Apwf9PG7zU1G21fRP4f8EuWAeAim?e=GJ4KzB

@TIHan
Copy link
Member

TIHan commented Nov 19, 2021

What I'll be curious about is how #11573 affected it.

@alfonsogarciacaro
Copy link
Contributor Author

@TIHan This was using v40.0.1-preview, so looking at the dates of the commits it should include that change. In any case, I'm unsure if #11573 would have much effect for Fable because in the first compilation it just executes an single call to ParseAndCheckProject: https://github.com/fable-compiler/Fable/blob/8446da056292b54b25b89fe6e5b378dd38de2e85/src/Fable.Cli/Main.fs#L340-L343

@dsyme
Copy link
Contributor

dsyme commented Mar 3, 2022

@alfonsogarciacaro I'm going to close this if that's ok, it's several months old

@dsyme dsyme closed this as completed Mar 3, 2022
@alfonsogarciacaro
Copy link
Contributor Author

Yes, unfortunately it looks the issue is not easily actionable. Did the profiling data help? Most of the performance decrease solved when switching back to ncave's fork, so I still suspect it has to do with IncrementalBuild.fs (that's the main difference between ncave's service_slim and the regular FCS API) though I couldn't confirm it when inspecting the profiling data (I don't have much experience analyzing profiling data so I may be missing something).

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

No branches or pull requests

5 participants