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

crypto, x/crypto: apply golang.org/wiki/TargetSpecific policy #31470

Open
FiloSottile opened this issue Apr 15, 2019 · 3 comments
Open

crypto, x/crypto: apply golang.org/wiki/TargetSpecific policy #31470

FiloSottile opened this issue Apr 15, 2019 · 3 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@FiloSottile
Copy link
Contributor

FiloSottile commented Apr 15, 2019 •
edited
Loading

A lot of packages need to be adapted to follow the new https://golang.org/wiki/TargetSpecific policy.

In particular, point 1 would make the generic code always available for testing, fuzzing, and analysis like #31456 (comment).
@FiloSottile FiloSottile added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Apr 15, 2019
@FiloSottile FiloSottile added this to the Go1.13 milestone Apr 15, 2019
@FiloSottile FiloSottile changed the title crypto, x/crypto: apply golang.org/wiki/MultiTarget policy crypto, x/crypto: apply golang.org/wiki/TargetSpecific policy Apr 16, 2019
@yaminisridaran
Copy link

@FiloSottile I would like to help in this issue. Can you please explain me the issue.

@mmcloughlin mmcloughlin mentioned this issue Jun 5, 2019
Open
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/205861 mentions this issue: curve25519: update package structure per golang.org/wiki/TargetSpecific

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/185439 mentions this issue: internal/chacha20: refactor for readability and consistency

gopherbot pushed a commit to golang/crypto that referenced this issue Nov 7, 2019
@FiloSottile
curve25519: update package structure per golang.org/wiki/TargetSpecific
fe70532 
This was extracted from CL 205157.

Updates golang/go#31470

Change-Id: I8109b874d4c3897ffc920bb4e12c71836da9d44a
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/205861
Reviewed-by: Katie Hockman <katie@golang.org>
FiloSottile added a commit to FiloSottile/go that referenced this issue Nov 24, 2019
@FiloSottile
poly1305: rewrite the Go implementation with 64-bit limbs
2dbfe90 
The new code is meant to be readable without external references for
Poly1305, and explains the field logic. The generic code is now 30-50%
faster on a Intel(R) Xeon(R) CPU E5-2690 v3 @ 2.60GHz, and even better
on a 3.1 GHz i7 MacBook.

name        old time/op    new time/op    delta
64-48          126ns ± 0%      80ns ± 1%  -36.24%  (p=0.000 n=16+20)
1K-48         1.07µs ± 0%    0.81µs ± 2%  -23.63%  (p=0.000 n=19+20)
2M-48         2.07ms ± 0%    1.61ms ± 1%  -22.31%  (p=0.000 n=20+20)
Write64-48    79.3ns ± 0%    58.0ns ± 1%  -26.89%  (p=0.000 n=20+19)
Write1K-48    1.02µs ± 0%    0.79µs ± 1%  -22.91%  (p=0.000 n=19+19)
Write2M-48    2.07ms ± 0%    1.61ms ± 2%  -22.33%  (p=0.000 n=17+20)

name        old speed      new speed      delta
64-48        508MB/s ± 0%   797MB/s ± 1%  +56.95%  (p=0.000 n=16+20)
1K-48        960MB/s ± 0%  1257MB/s ± 2%  +30.94%  (p=0.000 n=18+20)
2M-48       1.01GB/s ± 0%  1.30GB/s ± 1%  +28.73%  (p=0.000 n=20+20)
Write64-48   807MB/s ± 0%  1104MB/s ± 1%  +36.78%  (p=0.000 n=18+19)
Write1K-48  1.00GB/s ± 0%  1.30GB/s ± 1%  +29.71%  (p=0.000 n=18+19)
Write2M-48  1.01GB/s ± 0%  1.31GB/s ± 2%  +28.77%  (p=0.000 n=17+20)

The assembly is still 50-90% faster on the Xeon, 30-60% on the MacBook.
The Go code does not use all the arithmetic tricks the assembly does,
and it does not have access to the three operand wide shift instruction.

name        old time/op    new time/op    delta
64-48         80.3ns ± 1%    54.2ns ± 0%  -32.50%  (p=0.000 n=20+17)
1K-48          815ns ± 2%     446ns ± 1%  -45.27%  (p=0.000 n=20+20)
2M-48         1.61ms ± 1%    0.86ms ± 0%  -46.54%  (p=0.000 n=20+17)
Write64-48    58.0ns ± 1%    34.0ns ± 0%  -41.34%  (p=0.000 n=19+20)
Write1K-48     790ns ± 1%     427ns ± 0%  -45.92%  (p=0.000 n=19+17)
Write2M-48    1.61ms ± 2%    0.86ms ± 0%  -46.51%  (p=0.000 n=20+20)

name        old speed      new speed      delta
64-48        797MB/s ± 1%  1180MB/s ± 0%  +48.09%  (p=0.000 n=20+19)
1K-48       1.26GB/s ± 2%  2.30GB/s ± 1%  +82.71%  (p=0.000 n=20+20)
2M-48       1.30GB/s ± 1%  2.44GB/s ± 0%  +87.04%  (p=0.000 n=20+17)
Write64-48  1.10GB/s ± 1%  1.88GB/s ± 0%  +70.52%  (p=0.000 n=19+18)
Write1K-48  1.30GB/s ± 1%  2.40GB/s ± 0%  +84.84%  (p=0.000 n=19+18)
Write2M-48  1.31GB/s ± 2%  2.44GB/s ± 0%  +86.93%  (p=0.000 n=20+20)

Hopefully this will also avoid the need for an arm64 implementation.

Since now the Go and the amd64/ppc64le assembly use the same limb
schedule, drop the assembly initialize and finalize implementations,
and make the wrapper code match. It comes with a minor slowdown.

name        old time/op    new time/op    delta
64-48         50.3ns ± 0%    54.2ns ± 0%  +7.73%  (p=0.000 n=20+17)
1K-48          441ns ± 0%     446ns ± 1%  +1.10%  (p=0.000 n=19+20)
2M-48          860µs ± 0%     859µs ± 0%    ~     (p=0.178 n=19+17)
Write64-48    34.0ns ± 0%    34.0ns ± 0%    ~     (all equal)
Write1K-48     424ns ± 0%     427ns ± 0%  +0.71%  (p=0.000 n=17+17)
Write2M-48     860µs ± 0%     859µs ± 0%  -0.04%  (p=0.000 n=19+20)

name        old speed      new speed      delta
64-48       1.27GB/s ± 0%  1.18GB/s ± 0%  -7.20%  (p=0.000 n=20+19)
1K-48       2.32GB/s ± 0%  2.30GB/s ± 1%  -1.07%  (p=0.000 n=18+20)
2M-48       2.44GB/s ± 0%  2.44GB/s ± 0%    ~     (p=0.173 n=19+17)
Write64-48  1.88GB/s ± 0%  1.88GB/s ± 0%  +0.04%  (p=0.000 n=19+18)
Write1K-48  2.41GB/s ± 0%  2.40GB/s ± 0%  -0.67%  (p=0.000 n=19+18)
Write2M-48  2.44GB/s ± 0%  2.44GB/s ± 0%  +0.04%  (p=0.000 n=19+20)

Since poly1305/sum_generic.go was almost entirely rewritten, it's
probably best reviewed on gitiles.

This is the implementation published at
https://blog.filippo.io/a-literate-go-implementation-of-poly1305/

Updates golang#31470

Change-Id: I74f9011d3ee317a43b05ae7f05d96081d08bffd3
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/169037
Reviewed-by: Katie Hockman <katie@golang.org>
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.15 Mar 31, 2020
@katiehockman katiehockman removed their assignment Apr 20, 2020
@katiehockman katiehockman modified the milestones: Go1.15, Backlog Apr 20, 2020
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@FiloSottile
curve25519: update package structure per golang.org/wiki/TargetSpecific
8d85636 
This was extracted from CL 205157.

Updates golang/go#31470

Change-Id: I8109b874d4c3897ffc920bb4e12c71836da9d44a
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/205861
Reviewed-by: Katie Hockman <katie@golang.org>
c-expert-zigbee added a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@c-expert-zigbee
curve25519: update package structure per golang.org/wiki/TargetSpecific
a9b6f52 
This was extracted from CL 205157.

Updates golang/go#31470

Change-Id: I8109b874d4c3897ffc920bb4e12c71836da9d44a
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/205861
Reviewed-by: Katie Hockman <katie@golang.org>
c-expert-zigbee added a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@c-expert-zigbee
curve25519: update package structure per golang.org/wiki/TargetSpecific
7acce10 
This was extracted from CL 205157.

Updates golang/go#31470

Change-Id: I8109b874d4c3897ffc920bb4e12c71836da9d44a
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/205861
Reviewed-by: Katie Hockman <katie@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@FiloSottile
curve25519: update package structure per golang.org/wiki/TargetSpecific
0909239 
This was extracted from CL 205157.

Updates golang/go#31470

Change-Id: I8109b874d4c3897ffc920bb4e12c71836da9d44a
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/205861
Reviewed-by: Katie Hockman <katie@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@FiloSottile
curve25519: update package structure per golang.org/wiki/TargetSpecific
56c9297 
This was extracted from CL 205157.

Updates golang/go#31470

Change-Id: I8109b874d4c3897ffc920bb4e12c71836da9d44a
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/205861
Reviewed-by: Katie Hockman <katie@golang.org>
desdeel2d0m added a commit to desdeel2d0m/crypto that referenced this issue Jul 1, 2024
@desdeel2d0m @FiloSottile
curve25519: update package structure per golang.org/wiki/TargetSpecific
d538f90 
This was extracted from CL 205157.

Updates golang/go#31470

Change-Id: I8109b874d4c3897ffc920bb4e12c71836da9d44a
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/205861
Reviewed-by: Katie Hockman <katie@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@rsc @andybons @FiloSottile @gopherbot @yaminisridaran @katiehockman