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

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

Merged
merged 15 commits into from Aug 27, 2017

Conversation

Projects
None yet
4 participants
@matthid
Member

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

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 26, 2017

Member

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 :/

Member

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

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 26, 2017

Member

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

Member

matthid commented Aug 26, 2017

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

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 26, 2017

Member

Oh my god this seems to work

Member

matthid commented Aug 26, 2017

Oh my god this seems to work

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 26, 2017

Member
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.
Member

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 matthid changed the title from [WIP] make restore fast by saving a hash of the lockfile. to make restore fast by saving a hash of the lockfile. Aug 26, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 26, 2017

Member

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

Member

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 from make restore fast by saving a hash of the lockfile. to make restore fast by copying and comparing the lockfile. Aug 26, 2017

@matthid matthid changed the title from make restore fast by copying and comparing the lockfile. to [WIP] make restore fast by copying and comparing the lockfile. Aug 26, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 26, 2017

Member

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

Member

matthid commented Aug 26, 2017

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

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 27, 2017

Member

@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?

Member

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 from [WIP] make restore fast by copying and comparing the lockfile. to make restore fast by copying and comparing the lockfile. Aug 27, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 27, 2017

Member

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

Member

matthid commented Aug 27, 2017

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

referencesFile.Groups
|> Seq.collect (fun kv -> kv.Value.NugetPackages)
|> Seq.map (fun i -> i.Name)
|> Set.ofSeq

This comment has been minimized.

@matthid

matthid Aug 27, 2017

Member

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 Aug 27, 2017

Member

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)

static member Init(directory) = Dependencies.Init(directory,false)
/// Initialize paket.dependencies file in the given directory
static member Init(directory,fromBootstrapper) =

This comment has been minimized.

@matthid

matthid Aug 27, 2017

Member

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

@matthid

matthid Aug 27, 2017

Member

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

@alfonsogarciacaro

This comment has been minimized.

Show comment
Hide comment
@alfonsogarciacaro

alfonsogarciacaro Aug 27, 2017

@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.

alfonsogarciacaro commented Aug 27, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 27, 2017

Member

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

Member

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 Aug 27, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 27, 2017

Member

TLDR:

  • This PR improves the speed of paket restore
    • by saving "cache" files and quickly checking if they are dirty
    • by bypassing Argu (see fsprojects/Argu#90)
  • 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)
Member

matthid commented Aug 27, 2017

TLDR:

  • This PR improves the speed of paket restore
    • by saving "cache" files and quickly checking if they are dirty
    • by bypassing Argu (see fsprojects/Argu#90)
  • 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)
@alfonsogarciacaro

This comment has been minimized.

Show comment
Hide comment
@alfonsogarciacaro

alfonsogarciacaro Aug 27, 2017

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

alfonsogarciacaro commented Aug 27, 2017

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

matthid added some commits Aug 27, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 27, 2017

Member

@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 :)

Member

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

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 27, 2017

Member

Ok green - finally :)

Member

matthid commented Aug 27, 2017

Ok green - finally :)

@matthid matthid merged commit cb4cd32 into master Aug 27, 2017

1 of 4 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 27, 2017

Member

@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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 28, 2017

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@alfonsogarciacaro

alfonsogarciacaro Aug 28, 2017

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

alfonsogarciacaro commented Aug 28, 2017

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

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 28, 2017

Member

@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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@forki

forki Aug 28, 2017

Member
Member

forki commented Aug 28, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 28, 2017

Member

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

Member

matthid commented Aug 28, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment