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

Fix multiple sources (performance) #2499

Merged
merged 9 commits into from Jul 10, 2017

Conversation

Projects
None yet
3 participants
@matthid
Member

matthid commented Jul 9, 2017

Fixes #2369

Main problem was using the v2 api for myget which seems to be painfully slow. We always used v2 for all sources besides nuget, even when v3 uri was specified.
@forki was this intended?

Solves

source https://dotnet.myget.org/F/dotnet-core/api/v3/index.json
source https://dotnet.myget.org/F/cli-deps/api/v3/index.json
source https://www.nuget.org/api/v2/

nuget FSharp.Core >= 4.1.0
nuget FSharp.NET.Sdk >= 1.0.0

in

Performance:
 - Resolver: 46 seconds (1 runs)
    - Runtime: 8 seconds
    - Blocked (retrieving package versions): 25 seconds (6 times)
    - Blocked (retrieving package details): 12 seconds (33 times)
    - Not Blocked (retrieving package details): 36 times
    - Not Blocked (retrieving package versions): 63 times
 - Disk IO: 10 seconds
 - Average Request Time: 1 second
 - Number of Requests: 232

I also added other performance improvements which brought the runtime down to 4 Minutes (instead of hours), but switching to myget was the final breakthrough:

  • Properly cancel requests when we no longer need the result by delegating the cancellation token
  • Do GetVersion requests of a SINGLE source sequentially (because we should already put the fastest one first in the common case -> fewer requests)
  • Switch to System.Net.Http instead of WebRequest to have a single code-base for netcore and full.
  • Add messages to the install-process to make it more obvious what paket is doing
    The install-process is taking more and more time and users are wondering what paket is doing so I added some output

matthid added some commits Jul 9, 2017

Improve speed of myget
- Remove magic strings for known nuget sources
- Improve error handling of new system.net.http library
- Do not convert v3 sources to v2 sources when parsing them -> this lead
  to a huge performance bottleneck for myget.
@@ -308,7 +309,6 @@ let normalizeFeedUrl (source:string) =
| "http://nuget.org/api/v2" -> Constants.DefaultNuGetStream.Replace("https","http")
| "https://www.nuget.org/api/v2" -> Constants.DefaultNuGetStream
| "http://www.nuget.org/api/v2" -> Constants.DefaultNuGetStream.Replace("https","http")
| url when url.EndsWith("/api/v3/index.json") -> url.Replace("/api/v3/index.json","")

This comment has been minimized.

@matthid

matthid Jul 9, 2017

Member

This was a problem...

@matthid

matthid Jul 9, 2017

Member

This was a problem...

@matthid matthid changed the title from [WIP] Fix multiple sources (performance) to Fix multiple sources (performance) Jul 9, 2017

"",parserF ""
let! text,depsFile = async {
// TODO: Fixme, something is wrong on testcase #1341
if url <> "file://" then

This comment has been minimized.

@matthid

matthid Jul 9, 2017

Member

I think this was broken ever since... Something fishy is going on when paket tries to find the paket.dependencies file from a http file:///C:/.... entry. This was noticed, because HttpClient no longer supports file:/// and I could see in the debugger that this code tries to download file:// which looks horribly wrong. But as this is not related to this PR I added this "hack" as I have no idea what this is supposed to do.

@matthid

matthid Jul 9, 2017

Member

I think this was broken ever since... Something fishy is going on when paket tries to find the paket.dependencies file from a http file:///C:/.... entry. This was noticed, because HttpClient no longer supports file:/// and I could see in the debugger that this code tries to download file:// which looks horribly wrong. But as this is not related to this PR I added this "hack" as I have no idea what this is supposed to do.

@forki forki merged commit 25f3129 into master Jul 10, 2017

2 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@forki forki deleted the fix_multiple_sources branch Jul 10, 2017

@mnmr

This comment has been minimized.

Show comment
Hide comment
@mnmr

mnmr Aug 7, 2017

I'm wondering if the issue still remains considering that Paket (5.84.0) takes 1m 40s to check for outdated packages (and it reports almost that amount of time as being blocked, whatever that means exactly).

mnmr commented Aug 7, 2017

I'm wondering if the issue still remains considering that Paket (5.84.0) takes 1m 40s to check for outdated packages (and it reports almost that amount of time as being blocked, whatever that means exactly).

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 8, 2017

Member

@mnmr do you say we regressed on this depsfile? From the timing I'd say the reason are the fallbacks recently introduced but that means it should be resolved again with the fix warnings pr...

If you mean a different depsfile please open an issue to discuss...

Member

matthid commented Aug 8, 2017

@mnmr do you say we regressed on this depsfile? From the timing I'd say the reason are the fallbacks recently introduced but that means it should be resolved again with the fix warnings pr...

If you mean a different depsfile please open an issue to discuss...

@mnmr

This comment has been minimized.

Show comment
Hide comment
@mnmr

mnmr Aug 8, 2017

I'm afraid I'm not in a position where I can answer that - I only just downloaded Paket yesterday, so do not have any reference point for earlier behaviors. I just observed that it was NuGet-like slow ;) and came across this issue while trying to find out if this was expected or not (which I'm still not entirely sure about).

mnmr commented Aug 8, 2017

I'm afraid I'm not in a position where I can answer that - I only just downloaded Paket yesterday, so do not have any reference point for earlier behaviors. I just observed that it was NuGet-like slow ;) and came across this issue while trying to find out if this was expected or not (which I'm still not entirely sure about).

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