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

add a way to efficiently map many project options to FCS options at once #156

Merged
merged 2 commits into from
Aug 4, 2022

Conversation

baronfel
Copy link
Collaborator

@baronfel baronfel commented Jul 23, 2022

Closes #155 by providing an all-in-one mapping function in addition to the one-shot mapping function. Would like to grab some of the dense tests from dotnet/fsharp to verify before merging.

@baronfel baronfel force-pushed the map-many-fcs-options branch from 97f4365 to bfcb0f1 Compare July 23, 2022 19:12
@safesparrow
Copy link
Contributor

safesparrow commented Aug 2, 2022

Any chance to get this merged and released @baronfel ?

I used a custom version fsharp-benchmark-generator to run a test and show this does work as expected.
Generator code: https://github.com/safesparrow/fsharp-benchmark-generator/tree/projinfo-update
It uses the new FCS.mapManyProjects function.

You can use the following script to reproduce the test:
# Set the path to the local ProjInfo project
$projInfoPath = "C:\projekty\proj-info\src\Ionide.ProjInfo.FCS\Ionide.ProjInfo.FCS.fsproj"
# Set the path to where the git checkouts should be stored. Must be short on Windows due to long path issues.
$artifactPath = c:/.artifacts

# Checkout benchmark generator
git clone https://github.com/safesparrow/fsharp-benchmark-generator
Push-Location fsharp-benchmark-generator
git checkout projinfo-update
dotnet build .\Benchmarks.Generator.fsproj /p:ProjInfoProjectPath=$projInfoPath

dotnet run --no-build -- -i .\inputs\stress_big_dense.json --dry-run -n 1 -v -c $artifactPath

Pop-Location

The relevant output should look something like this, indicating that the FSharpOptions generation took a very short time:

[01:17:42 INF] LoadOptions: 30 projects loaded from c:\.artifacts\stress_big_dense\a7b668136cc720e50b92b0cae0f06081c5df1117\tests/projects/stress/big_netstandard/dense/Dense.sln
[01:17:42 VRB] PrepareAndRun: Generating actions

I do have one request for the PR:

  • Can we make FCS.mapToFSharpProjectOptions use the same caching mechanism? The problem occurs always when the project graph is not a tree. It doesn't matter whether the user calls the function for each project independently or for the root project.

@baronfel
Copy link
Collaborator Author

baronfel commented Aug 2, 2022

I was thinking about the method a bit: I worried about making it use the same caching mechanism because it would allocate a new caching dictionary on every call. Maybe not the worst thing but it was a little concerning to me.

I think the other thing I was trying to do was figure out a good way to bring these two under test. Ideally I'd like to verify that we don't regress the perf.

@safesparrow
Copy link
Contributor

I was thinking about the method a bit: I worried about making it use the same caching mechanism because it would allocate a new caching dictionary on every call. Maybe not the worst thing but it was a little concerning to me.

Every call allocates a big number of large objects already. I really don't think that one temporary dictionary is something to worry about at all IMO.

Regarding perf: a simple test would be to construct a dense project graph on disk, process that and:

  1. Assert that the number of unique options objects equals the number of projects (as a proxy for whether a project was analysed multiple times).
  2. Inject a counter to the method and achieve the same thing more explicitly.

…aph doesn't contain duplicates (#159)

* Add a unit test for checking that FSharpProjectOptions are created once per project as a proxy for checking that every project is mapped once

* Fix test
@baronfel
Copy link
Collaborator Author

baronfel commented Aug 4, 2022

Thank you for your help @safesparrow!

@baronfel baronfel merged commit 273b581 into main Aug 4, 2022
@baronfel baronfel deleted the map-many-fcs-options branch August 4, 2022 23:20
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.

mapToFSharpProjectOptions for a dense project graph is very slow
2 participants