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 tweaks for larger solutions #210

Merged
merged 1 commit into from
May 15, 2024
Merged

Conversation

TheAngryByrd
Copy link
Member

@TheAngryByrd TheAngryByrd commented May 14, 2024

Some performance tweaks for larger solutions when using the CLI tool.

This took my work solution from 15+ minutes to about 2 minutes.

Comment on lines +43 to +52
if not (isNull formatterOptions.TimestampFormat) then
let dateTime =
if formatterOptions.UseUtcTimestamp then
DateTime.UtcNow
else
DateTime.Now

let timestamp = dateTime.ToString(formatterOptions.TimestampFormat)

textWriter.Write($"{timestamp} ")
Copy link
Member Author

Choose a reason for hiding this comment

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

Helped with diagnosing slow areas. Not exactly required for this but might be nice to keep.

Comment on lines +42 to +44
| Project _ -> "List of paths to your .fsproj file."
| Analyzers_Path _ ->
"List of path to a folder where your analyzers are located. This will search recursively."
Copy link
Member Author

Choose a reason for hiding this comment

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

Noticed these took lists and should give help messages appropriate for them.

let loader = WorkspaceLoader.Create(toolsPath, properties)
let parsed = loader.LoadProjects [ projPath ] |> Seq.toList
let projectOptions = loader.LoadProjects projPaths
Copy link
Member Author

@TheAngryByrd TheAngryByrd May 14, 2024

Choose a reason for hiding this comment

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

This is one of the major speed ups. Load all projects at once.

There's some locking that goes on behind the scenes when ProjInfo talks to msbuild becuase msbuild can only handle one build request at a time in process. It's best to front load as many project load requests as you can.


return fcsPo
return FCS.mapManyOptions projectOptions |> Seq.toList
Copy link
Member Author

Choose a reason for hiding this comment

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

Another helper that maps to FCS types efficiently.

}

let runProjectAux
let runProject
Copy link
Member Author

@TheAngryByrd TheAngryByrd May 14, 2024

Choose a reason for hiding this comment

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

runProjectAux was no longer needed, renamed to runProject


// Since we did ParseAndCheckProject, we can be sure that the file is in the project.
// See https://fsharp.github.io/fsharp-compiler-docs/fcs/project.html for more information.
let! parseAndCheckResults = fcs.GetBackgroundCheckResultsForFileInProject(fileName, fsharpOptions)
Copy link
Member Author

Choose a reason for hiding this comment

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

As the comment mentions, this helps with speedily getting the typecheck for that file since we already checked the project.

|> List.map (fun (projPath: FSharpProjectOptions) ->
runProject client projPath exclInclFiles severityMapping
)
|> Async.Parallel
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we front load all projects we can use Async.Parallel to schedule as all the operations.

@TheAngryByrd TheAngryByrd marked this pull request as ready for review May 14, 2024 15:30
@nojaf
Copy link
Contributor

nojaf commented May 15, 2024

@TheAngryByrd could you retry syncing with the main branch? I believe stuff will work after that.

@baronfel baronfel merged commit bee7ea7 into main May 15, 2024
2 checks passed
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