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

improved restore speedup #3512

Merged
merged 5 commits into from Feb 22, 2019

Conversation

Projects
None yet
4 participants
@viktor-svub
Copy link
Contributor

commented Feb 21, 2019

hi everyone and sorry for the long silence...

  • this change improves performance of paket restore, specifically where way too many inter-connected dependencies are involved
  • we have projects depending on the majority of the AspNetCore.App bundle, multi-targeting net462, netstandard2.0, and netcoreapp2.1
  • paket restore and/or dotnet restore take tens of seconds, easily over minute, per project/platform combination
  • with ANTS profiler, I identified the result-generating loop of LockFile's GetOrderedPackageHull, as it was re-creating a lot of Set instances
  • after rewriting the loop to re/use mutable state, the performance improved at least 20 times, taking approx 2 seconds for the whole solution

viktor-svub added some commits Aug 4, 2018

Merge upstream master:6ec1622 from fsprojects/Paket
from origin (Merge pull request #3323 from fsprojects/matthid-patch-1)
@isaacabraham

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Mutability for the win!

@baronfel

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Better yet, profiling + judicious use of localized mutability 👍 Nice work @viktor-svub!

@forki

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

@viktor-svub

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Well, I could change them as well, but their operations did not light up under the profiler and I generally avoid touching working code -- using F# Set there does not hurt, so from my PoV it's the preferred way.

I suspect the inherently expensive slow operation here is Set.map (which seems to always enforce re/creation of new set), and rest of the refactoring was only done to keep the code readable, as an attempt to replace just the map/filter and keep the rest semi-immutable did not end well.

@forki

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

ok thanks

@forki forki merged commit 82c341a into fsprojects:master Feb 22, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.