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.

Sign up for GitHub

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

Jump to bottom

make restore fast by copying and comparing the lockfile. #2675

Merged
merged 15 commits into from
Aug 27, 2017
Merged

make restore fast by copying and comparing the lockfile. #2675

merged 15 commits into from
Aug 27, 2017

Conversation

matthid
Copy link
Member

@matthid matthid commented Aug 26, 2017

I think checking & validating this hash in our msbuild targets file should make building large paket based solutions a lot faster...

@matthid
Copy link
Member Author

matthid commented Aug 26, 2017

I'm really wondering that this in itself doesn't make it faster at all (3 instead of 5 secs).

From my first profiling attempt it looks like Argu is taking the most time :/

@matthid
Copy link
Member Author

matthid commented Aug 26, 2017

Also the magic mode bootstrapper is probably eating quite some time there

0x53A
0x53A reviewed Aug 26, 2017
View reviewed changes
src/Paket/Program.fs Outdated
try
let args = Environment.GetCommandLineArgs()
if args.Length = 1 && args.[0] = "restore" || args.Length = 2 && args.[0] = "--from-bootstrapper" && args.[1] = "restore" then
Copy link
Contributor

Choose a reason for hiding this comment

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

match args with
| [| "restore" |]
| [| "--from-bootstrapper" ; "restore" |]

?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes looks better but might be slower :P
The main problem is generating the sha512 hash in msbuild anyway ;) I don't think what I want is even possible...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok fixed :)

@matthid
Copy link
Member Author

matthid commented Aug 26, 2017

Oh my god this seems to work

@matthid
Copy link
Member Author

matthid commented Aug 26, 2017 

matth@DESKTOP-8VDEORF MINGW64 /c/proj/Paket (restore_speed)
$ PaketExePath=/c/proj/Paket/bin/paket.exe dotnet restore src/Paket.Core.preview3/Paket.Core.fsproj
  Paket version 5.91.0
  Performance:
   - Disk IO: 419 milliseconds
   - Runtime: 3 seconds
  Restore completed in 33.38 ms for C:\proj\Paket\src\Paket.Core.preview3\Paket.Core.fsproj.

matth@DESKTOP-8VDEORF MINGW64 /c/proj/Paket (restore_speed)
$ PaketExePath=/c/proj/Paket/bin/paket.exe dotnet restore src/Paket.Core.preview3/Paket.Core.fsproj
  Restore completed in 35.97 ms for C:\proj\Paket\src\Paket.Core.preview3\Paket.Core.fsproj.

matthid added a commit to fsprojects/Paket.VisualStudio that referenced this pull request Aug 26, 2017
@matthid
only use 'restore all' to benefit from fsprojects/Paket#2675
593f09d
@matthid matthid changed the title [WIP] make restore fast by saving a hash of the lockfile. make restore fast by saving a hash of the lockfile. Aug 26, 2017
@matthid
Copy link
Member Author

matthid commented Aug 26, 2017

IMHO this is good enough for now. It will hopefully help large netcore solutions and Paket.VisualStudio

@matthid matthid changed the title make restore fast by saving a hash of the lockfile. make restore fast by copying and comparing the lockfile. Aug 26, 2017
@matthid matthid changed the title make restore fast by copying and comparing the lockfile. [WIP] make restore fast by copying and comparing the lockfile. Aug 26, 2017
@matthid
Copy link
Member Author

matthid commented Aug 26, 2017

WIP again, because netcore integration now needs to generate all files for all relevant frameworks beforehand

@matthid
Copy link
Member Author

matthid commented Aug 27, 2017

@alfonsogarciacaro I'm trying to change the restore process here a bit. In the future paket will write "obj",projectFile.Name "." targetFramework ".references" instead of "obj",projectFile.Name ".references". I know that Fable uses this file.

We can still write the old file for compat. The question is what should we write into it? What is the target-framework fable uses? netstandard16?

@matthid matthid changed the title [WIP] make restore fast by copying and comparing the lockfile. make restore fast by copying and comparing the lockfile. Aug 27, 2017
@matthid
Copy link
Member Author

matthid commented Aug 27, 2017

I guess the build will be red because this is a bootstrapping problem :/

matthid
matthid commented Aug 27, 2017
View reviewed changes
src/Paket.Core/PublicAPI.fs
referencesFile.Groups
|> Seq.collect (fun kv -> kv.Value.NugetPackages)
|> Seq.map (fun i -> i.Name)
|> Set.ofSeq
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 might be a slight change in behavior. I just look into the references file and assume all things there are direct deps (which might not be true but I planned to add a warning when transitives are listed in the references file - should be issue open somewhere)

matthid
matthid commented Aug 27, 2017
View reviewed changes
src/Paket.Core/PublicAPI.fs
static member Init(directory) = Dependencies.Init(directory,false)

/// Initialize paket.dependencies file in the given directory
static member Init(directory,fromBootstrapper) =
Copy link
Member Author

Choose a reason for hiding this comment

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

While technically a breaking change in the public API - I don't think anyone used these weird overloads.

@alfonsogarciacaro
Copy link

@matthid Fable 1.1.x uses netstandard1.6, I already publish a beta of Fable 1.2 which will use netstandard2.0, but I can make so the final Fable 1.2 looks for "obj",projectFile.Name "." targetFramework ".references". So, if we want to keep compatibility with Fable 1.1.x for a time, it should be fine to use netstandard1.6.

@matthid
Copy link
Member Author

matthid commented Aug 27, 2017

@alfonsogarciacaro Thanks will update. So I will also assume that if no netstandard16 is in the project file we no longer need that file.

@matthid matthid requested a review from forki August 27, 2017 13:49
@matthid
Copy link
Member Author

matthid commented Aug 27, 2017

TLDR:

  • This PR improves the speed of paket restore
  • This PR skips calling paket restore completely if we can detect in MSBuild that we are still up2date. (by using the same caching files as above)

@matthid matthid mentioned this pull request Aug 27, 2017
Open
@alfonsogarciacaro
Copy link

Awesome work, @matthid! Could you please check if this is going in the right direction? fable-compiler/Fable@112fcd3#diff-e47dbd37b262f2f0394b5704369d4b31R109

@matthid
Copy link
Member Author

matthid commented Aug 27, 2017

@alfonsogarciacaro Yes that looks exactly like it would do. Currently I'm thinking about renaming it to end with .paket.references but I haven't decided anything yet. Will let you know if I do :)

@matthid
Copy link
Member Author

matthid commented Aug 27, 2017

Ok green - finally :)

@matthid matthid merged commit cb4cd32 into master Aug 27, 2017
@matthid
Copy link
Member Author

matthid commented Aug 27, 2017

@Krzysztof-Cieslak Is Ionide calling paket restore directly or only dotnet restore? If the former we might want to take a look to benefit from this as well. I already updated fsprojects/Paket.VisualStudio#140 to benefit as well.

@matthid
Copy link
Member Author

matthid commented Aug 28, 2017

Results in FAKE:

Before:

DotnetRestore                      00:05:35.9467550
DotnetPackage                      00:06:52.9450041

https://ci.appveyor.com/project/matthid/fake-6w516/build/1.0.106

After:

DotnetRestore                       0 <- no longer required
DotnetPackage                       00:04:32.9611943

https://ci.appveyor.com/project/matthid/fake-6w516/build/1.0.148

@alfonsogarciacaro
Copy link

Awesome! How is the suffix finally? .references or .paket.references? When will this be released?

@matthid matthid mentioned this pull request Aug 28, 2017
Open
@matthid
Copy link
Member Author

matthid commented Aug 28, 2017

@alfonsogarciacaro Yes I think it makes sense, will try adding it later today. And I think this can be released. But before that someone (@forki) needs to answer how the target files are updated. Because older target files should be compatible with newer paket.exe but definitely not the other way around (So we need to know if this can happen)

Once we know that we can make a "real" release with this.

@forki
Copy link
Member

forki commented Aug 28, 2017 via email

@matthid
Copy link
Member Author

matthid commented Aug 28, 2017

Perfect, thanks that should work. Will add another PR with the renaming later.

@matthid matthid deleted the restore_speed branch July 2, 2019 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0x53A 0x53A 0x53A left review comments

@forki forki Awaiting requested review from forki

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matthid @alfonsogarciacaro @forki @0x53A