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

crypto/tls: drop in BenchmarkThroughput performance #17779

Closed
mundaym opened this issue Nov 3, 2016 · 3 comments
Closed

crypto/tls: drop in BenchmarkThroughput performance #17779

mundaym opened this issue Nov 3, 2016 · 3 comments
Assignees
Milestone

Comments

@mundaym
Copy link
Member

@mundaym mundaym commented Nov 3, 2016

What version of Go are you using (go version)?

go version devel +4141054 Thu Nov 3 17:42:01 2016 +0000 darwin/amd64

What did you do?

go test crypto/tls -bench BenchmarkThroughput

What did you expect to see?

Similar performance to 1.7.1.

What did you see instead?

A slowdown from 1.7.1 (apologies for the noisy benchmarks, this is on my laptop):

name                           old speed     new speed     delta
Throughput/MaxPacket/1MB       137MB/s ±25%  132MB/s ± 8%     ~     (p=0.529 n=10+10)
Throughput/MaxPacket/2MB       162MB/s ±12%  141MB/s ±23%  -13.07%  (p=0.019 n=10+10)
Throughput/MaxPacket/4MB       187MB/s ±23%  158MB/s ±15%  -15.16%  (p=0.007 n=10+10)
Throughput/MaxPacket/8MB       213MB/s ±12%  172MB/s ± 6%  -19.16%  (p=0.000 n=10+9)
Throughput/MaxPacket/16MB      226MB/s ±23%  178MB/s ±20%  -21.11%  (p=0.002 n=10+10)
Throughput/MaxPacket/32MB      238MB/s ± 7%  166MB/s ±22%  -30.32%  (p=0.000 n=10+10)
Throughput/MaxPacket/64MB      228MB/s ±13%  182MB/s ±12%  -20.27%  (p=0.000 n=10+10)
Throughput/DynamicPacket/1MB   137MB/s ±13%  108MB/s ±29%  -21.18%  (p=0.002 n=10+10)
Throughput/DynamicPacket/2MB   160MB/s ±23%  142MB/s ± 7%     ~     (p=0.143 n=10+10)
Throughput/DynamicPacket/4MB   185MB/s ±11%  149MB/s ±13%  -19.69%  (p=0.000 n=10+10)
Throughput/DynamicPacket/8MB   224MB/s ± 7%  174MB/s ±16%  -22.41%  (p=0.000 n=9+10)
Throughput/DynamicPacket/16MB  226MB/s ±18%  177MB/s ±20%  -21.48%  (p=0.001 n=10+10)
Throughput/DynamicPacket/32MB  227MB/s ±20%  169MB/s ±23%  -25.42%  (p=0.000 n=10+10)
Throughput/DynamicPacket/64MB  249MB/s ± 6%  182MB/s ±12%  -26.72%  (p=0.000 n=10+9)

It looks like (although I haven't verified it) this is because the default cipher suite in crypto/tls is now using ChaCha20/Poly1305 in preference to AES-GCM. I suspect this is fine (and is probably better for arm etc.), but I thought I'd open this issue in case it requires documentation or something.

@bradfitz bradfitz added this to the Go1.8 milestone Nov 3, 2016
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 3, 2016

Yeah, it's at least something for the release notes.

/cc @agl

@agl agl self-assigned this Nov 3, 2016
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 7, 2016

CL https://golang.org/cl/32871 mentions this issue.

@gopherbot gopherbot closed this in a9ce0f9 Nov 7, 2016
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 7, 2016

CL https://golang.org/cl/32838 mentions this issue.

gopherbot pushed a commit that referenced this issue Nov 7, 2016
CL 32871 updated the default cipher suites to use AES-GCM in
preference to ChaCha20-Poly1305 on platforms which have hardware
implementations of AES-GCM. This change makes BenchmarkThroughput
use the default cipher suites instead of the test cipher suites to
ensure that the recommended (fastest) algorithms are used.

Updates #17779.

Change-Id: Ib551223e4a00b5ea197d4d73748e1fdd8a47c32d
Reviewed-on: https://go-review.googlesource.com/32838
Run-TryBot: Michael Munday <munday@ca.ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
@golang golang locked and limited conversation to collaborators Nov 7, 2017
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
…re support is present.

Support for ChaCha20-Poly1305 ciphers was recently added to crypto/tls.
These ciphers are preferable in software, but they cannot beat hardware
support for AES-GCM, if present.

This change moves detection for hardware AES-GCM support into
cipher/internal/cipherhw so that it can be used from crypto/tls. Then,
when AES-GCM hardware is present, the AES-GCM cipher suites are
prioritised by default in crypto/tls. (Some servers, such as Google,
respect the client's preference between AES-GCM and ChaCha20-Poly1305.)

Fixes golang#17779.

Change-Id: I50de2be486f0b0b8052c4628d3e3205a1d54a646
Reviewed-on: https://go-review.googlesource.com/32871
Run-TryBot: Adam Langley <agl@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
…re support is present.

Support for ChaCha20-Poly1305 ciphers was recently added to crypto/tls.
These ciphers are preferable in software, but they cannot beat hardware
support for AES-GCM, if present.

This change moves detection for hardware AES-GCM support into
cipher/internal/cipherhw so that it can be used from crypto/tls. Then,
when AES-GCM hardware is present, the AES-GCM cipher suites are
prioritised by default in crypto/tls. (Some servers, such as Google,
respect the client's preference between AES-GCM and ChaCha20-Poly1305.)

Fixes golang#17779.

Change-Id: I50de2be486f0b0b8052c4628d3e3205a1d54a646
Reviewed-on: https://go-review.googlesource.com/32871
Run-TryBot: Adam Langley <agl@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.