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

[WIP] Parallel type checking for impl files with backing sig files - fsc.exe #11152

Closed
wants to merge 28 commits into from

Conversation

TIHan
Copy link
Member

@TIHan TIHan commented Feb 26, 2021

We sort of already do parallel type-checking in our tooling scenarios, namely when checking active documents. We need to prove that this can be done safely.

This PR allows parallel type-checking on implementation files that have a corresponding signature file. This is very similar to what we did in tooling where we skipped checking implementation files. Now, we just aggregate all the impl files that we skipped, then type-check them in parallel.

This optimization only benefits projects who use signature files heavily. Trying to parallelize impl files that do not have sig files is not possible due to type inference.

We need to be cautious with this type of change. We are effectively assuming that TypeCheckOneInput is safe to be done in parallel for a build.

Early results

CPU: Core i7-10700K @ 3.8GHz (8 cores, 16 logical cores)

msbuild FSharp.Compiler.Service.fsproj with only compiling netstandard2.0 target - this is also without generating the parser:

  • Parallel Off: Time Elapsed 00:00:38.65
  • Parallel On: Time Elapsed 00:00:26.10 - this includes parallel parsing

That's a whopping total ~30% decrease in compile time when re-building the compiler service with parallel enabled for parsing and type-checking.
On top of parallel parsing, it decreased time by ~23%.

msbuild FSharp.Core.fsproj:

  • Parallel Off: Time Elapsed 00:00:12.00
  • Parallel On: Time Elapsed 00:00:9.10

~25% total boost when compiling FSharp.Core.

PR Dependencies

Acceptance Criteria

  • Deep review to ensure there are no cases where concurrency problems exists.
    • As of this edit of the post, the compiler is now able to handle concurrent requests - therefore, we are already commonly running the type-checker in parallel in VS.
  • Verify determinism of the compiler with this change
  • Add performance tests

@TIHan TIHan changed the title [WIP][Experiment] Parallel type checking for impl files with backing sig files [WIP][Experiment] Parallel type checking for impl files with backing sig files - fsc.exe Feb 26, 2021

newResults |> List.ofArray, tcState
else
(tcState, inputs) ||> List.mapFold (TypeCheckOneInputSkipImpl (ctok, checkForErrors, tcConfig, tcImports, tcGlobals, prefixPathOpt))
Copy link
Member Author

Choose a reason for hiding this comment

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

This should not use the SkipImpl call.

@TIHan TIHan changed the title [WIP][Experiment] Parallel type checking for impl files with backing sig files - fsc.exe [WIP] Parallel type checking for impl files with backing sig files - fsc.exe Feb 26, 2021
@abelbraaksma
Copy link
Contributor

Really awesome stuff!

On the implementation files without signature files, if we could determine they're not dependent (I.e. file A doesn't have any opens to a module in file B, and no qualified names, which may be detected pre-typechecking), parallelization there might be possible at some point as well.

@TIHan
Copy link
Member Author

TIHan commented Feb 26, 2021

On the implementation files without signature files, if we could determine they're not dependent (I.e. file A doesn't have any opens to a module in file B, and no qualified names, which may be detected pre-typechecking), parallelization there might be possible at some point as well.

If there was a way to prove that files are not dependent on each other you could parallelize them. From what I know, the only way to really prove that is by type checking the whole file. You could do some really serious heuristics by looking at the syntax tree, but that starts to feel like a slippery slope.

There are other ideas where we could parallelize IL code-gen and/or we could do reference assembly generation as well to improve perf.

@TIHan TIHan changed the title [WIP] Parallel type checking for impl files with backing sig files - fsc.exe [WIP][Experiment] Parallel type checking for impl files with backing sig files - fsc.exe Feb 26, 2021
@TIHan
Copy link
Member Author

TIHan commented Feb 26, 2021

Some tests are failing from what looks like the ordering of when errors get logged.

@abelbraaksma
Copy link
Contributor

Some tests are failing from what looks like the ordering of when errors get logged.

Errors usually get logged in source order (regardless of the VS error window showing them alphabetically, unfortunately, I've a bug report somewhere about that). Keeping that order would be nice, as often, fixing the first, fixes the rest.

You could do some really serious heuristics by looking at the syntax tree, but that starts to feel like a slippery slope.

Agreed. Another way might be to delay type resolution but that requires an extra step where the 'type holes' need to be filled in at a later stage. That's probably similarly complex, though it would certainly allow more parallelization.

Using reference assemblies would benefit from getting a much wanted feature for free (currently, the compiler cannot generate them, unless I've missed a feature update). But I'm unsure how much that helps with parallelization.

@TIHan TIHan changed the title [WIP][Experiment] Parallel type checking for impl files with backing sig files - fsc.exe [WIP] Parallel type checking for impl files with backing sig files - fsc.exe Mar 12, 2021
@TIHan TIHan mentioned this pull request May 6, 2021
5 tasks
@TIHan TIHan mentioned this pull request May 12, 2021
@vzarytovskii
Copy link
Member

Covered by #13737

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

Successfully merging this pull request may close these issues.

None yet

4 participants