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

curl: disable HTTP/2 support #20639

Closed
wants to merge 1 commit into from
Closed

curl: disable HTTP/2 support #20639

wants to merge 1 commit into from
+1 −2

Conversation

fpletz
Copy link
Member

@fpletz fpletz commented Nov 23, 2016 •
edited
Loading

Motivation for this change

Most services like cache.nixos.org and nginx httpd servers are much slower serving files using HTTP/2 with curl/nghttp2. The reason is unknown but it's just annoying.

If you use curl --http1.1 the downloads are fast but with curl --http2 they are slow. nghttp2 ships with a download tool which yields the same performance as curl --http2. Using a HTTP/2 capable browser like Chromium you can observe fast download speeds.

Benchmarks
$ nix-shell -p nghttp2 curl
$ time curl --http2 https://hydra.mayflower.de/nar/rhxyhz1pj949njsdyqfbh5hnbynns9z3-openttd-1.6.1 > /dev/null
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 18.7M    0 18.7M    0     0  1440k      0 --:--:--  0:00:13 --:--:-- 1309k

real    0m13.324s
user    0m0.258s
sys     0m0.145s

$ time curl --http1.1 https://hydra.mayflower.de/nar/rhxyhz1pj949njsdyqfbh5hnbynns9z3-openttd-1.6.1 > /dev/null
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 18.7M    0 18.7M    0     0  4687k      0 --:--:--  0:00:04 --:--:-- 4693k

real    0m4.098s
user    0m0.215s
sys     0m0.109s

$ time nghttp https://hydra.mayflower.de/nar/rhxyhz1pj949njsdyqfbh5hnbynns9z3-openttd-1.6.1 > /dev/null

real    0m12.874s
user    0m0.134s
sys     0m0.100s

@fpletz fpletz added 0.kind: regression Something that worked before working no longer 1.severity: mass-rebuild This PR causes a large number of packages to rebuild labels Nov 23, 2016
@fpletz fpletz mentioned this pull request Nov 23, 2016
Closed
@fpletz fpletz force-pushed the fix/curl-disable-http2 branch from 230f30c to fd98f94 Compare November 23, 2016 01:13
@fpletz
curl: disable HTTP/2 support
effd6e0 
Most services like cache.nixos.org and nginx httpd servers are much slower
serving files using HTTP/2 with curl/nghttp2. The reason is unknown but
it's just annoying.
@fpletz fpletz force-pushed the fix/curl-disable-http2 branch from fd98f94 to effd6e0 Compare November 23, 2016 01:14
@domenkozar
Copy link
Member

I thought @edolstra said they're MUCH faster? Maybe we need to include NixOS/nix@90ad02b

@fpletz
Copy link
Member Author

fpletz commented Nov 23, 2016

The Nix 1.11 download-from-binary-cache.pl script just calls curl and this is the tool that is broken. So it's IMHO independant of Nix yet Nix suffers from it.

I believe the commit you mentioned is for the 1.12 branch where this script has been rewritten in C++. The commit message just indicates that narinfos are downloaded faster, probably due to request multiplexing, but doesn't say anything about bigger NARs.

@domenkozar
Copy link
Member

OK. Before we disable HTTP/2 support globally I'd like to understand the underlying problem.

@domenkozar
Copy link
Member

I can reproduce

$ \time curl --http2 https://cache.nixos.org/nar/1wyksagbra4m12z5qka02jnaylc9w93szg36yxm9h4c93bszr0ci.nar.xz > /dev/null
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 24.7M  100 24.7M    0     0  1143k      0  0:00:22  0:00:22 --:--:-- 1254k
0.16user 0.21system 0:22.18elapsed 1%CPU (0avgtext+0avgdata 6876maxresident)k
0inputs+0outputs (0major+546minor)pagefaults 0swaps

$ \time curl --http1.1 https://cache.nixos.org/nar/1wyksagbra4m12z5qka02jnaylc9w93szg36yxm9h4c93bszr0ci.nar.xz > /dev/null
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 24.7M  100 24.7M    0     0  5715k      0  0:00:04  0:00:04 --:--:-- 5866k
0.10user 0.12system 0:04.44elapsed 5%CPU (0avgtext+0avgdata 6992maxresident)k
0inputs+0outputs (0major+543minor)pagefaults 0swaps

@domenkozar
Copy link
Member

Trying to bump nghttp2 to see if performance improves.

@fpletz
Copy link
Member Author

fpletz commented Nov 23, 2016 •
edited
Loading

@domenkozar Already tried that. Even slower in my tests. :/

@domenkozar
Copy link
Member

About the same for me (3 samples).

@edolstra
Copy link
Member

Strong 👎 on disabling HTTP/2 because of an issue with one HTTP server (Cloudfront).

@domenkozar
Copy link
Member

@edolstra it's not just Cloudfront, also apache (see issue description).

@domenkozar
Copy link
Member

nghttp2/nghttp2#730

@fpletz
Copy link
Member Author

fpletz commented Nov 23, 2016

@domenkozar The patch curl/curl@a4d8888 mentioned there fixes curl performance for me. Should we just include that patch?

@domenkozar
Copy link
Member

This is going to be fixed in next curl version.

See curl/curl@a4d8888 and nghttp2/nghttp2#730 (comment)

I think it's best to wait on the next release.

@domenkozar
Copy link
Member

@fpletz sure :)

@domenkozar domenkozar closed this Nov 23, 2016
@fpletz fpletz deleted the fix/curl-disable-http2 branch November 23, 2016 13:12
@fpletz
Copy link
Member Author

fpletz commented Nov 23, 2016

@domenkozar Thanks for your help. 🍻

@edolstra
Copy link
Member

Oddly, curl with HTTP/2 works fine from an EC2 instance. But maybe that's also dependent on the window size.

@edolstra
Copy link
Member

The window patch does fix the problem on non-EC2.

fpletz added a commit that referenced this pull request Nov 26, 2016
@fpletz
curl: add upstream patch to fix HTTP2 performance issues
9007303 
See #20639. Patch has to be in nixpkgs because fetchurl depends on curl.
domenkozar pushed a commit that referenced this pull request Dec 28, 2016
@fpletz @domenkozar
curl: add upstream patch to fix HTTP2 performance issues
8845b74 
See #20639. Patch has to be in nixpkgs because fetchurl depends on curl.

(cherry picked from commit 9007303)
Signed-off-by: Domen Kožar <domen@dev.si>
domenkozar added a commit that referenced this pull request Dec 28, 2016
@domenkozar
curl: backport all changes to fix #20639
d68873d
@domenkozar
Copy link
Member

I've backported this to 16.09: 8845b74 d68873d

@domenkozar domenkozar mentioned this pull request Dec 28, 2016
Closed
adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 2024
@adrianpk
curl: add upstream patch to fix HTTP2 performance issues
3537fb3 
See NixOS#20639. Patch has to be in nixpkgs because fetchurl depends on curl.

(cherry picked from commit 9007303)
Signed-off-by: Domen Kožar <domen@dev.si>
adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 2024
@adrianpk
curl: backport all changes to fix NixOS#20639
65c5cf3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
0.kind: regression Something that worked before working no longer 1.severity: mass-rebuild This PR causes a large number of packages to rebuild
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fpletz @domenkozar @edolstra