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

go: upgrade to 1.21 #4516

Merged
merged 2 commits into from
Sep 7, 2023
Merged

go: upgrade to 1.21 #4516

merged 2 commits into from
Sep 7, 2023

Conversation

sluongng
Copy link
Contributor

@sluongng sluongng commented Aug 9, 2023

For more information: https://go.dev/blog/go1.21

Related issues: N/A

@sluongng
Copy link
Contributor Author

sluongng commented Aug 9, 2023

We should wait for rules_go to confirm the compatibility first bazel-contrib/rules_go#3643

@bduffany
Copy link
Member

bduffany commented Aug 9, 2023

Super excited for speed boosts in crypto/sha256 🚀 would be cool to do a before/after benchmark of some functions like digest.Compute etc.

@bduffany
Copy link
Member

bduffany commented Aug 9, 2023

added a benchmark in #4518

before this PR

BenchmarkDigestCompute/SHA256/1-8                2624952               455.7 ns/op
BenchmarkDigestCompute/BLAKE3/1-8                 713340              1601 ns/op
BenchmarkDigestCompute/SHA256/10-8               2636890               463.4 ns/op
BenchmarkDigestCompute/BLAKE3/10-8                812553              1558 ns/op
BenchmarkDigestCompute/SHA256/100-8              2019846               587.7 ns/op
BenchmarkDigestCompute/BLAKE3/100-8               904671              2409 ns/op
BenchmarkDigestCompute/SHA256/1000-8              515154              2291 ns/op
BenchmarkDigestCompute/BLAKE3/1000-8              546352              3174 ns/op
BenchmarkDigestCompute/SHA256/10000-8              55933             19288 ns/op
BenchmarkDigestCompute/BLAKE3/10000-8             238008              8266 ns/op
BenchmarkDigestCompute/SHA256/100000-8              6258            188327 ns/op
BenchmarkDigestCompute/BLAKE3/100000-8             49518             23969 ns/op

after

BenchmarkDigestCompute/SHA256/1-8                2532780               459.6 ns/op
BenchmarkDigestCompute/BLAKE3/1-8                 786246              1413 ns/op
BenchmarkDigestCompute/SHA256/10-8               2640088               467.8 ns/op
BenchmarkDigestCompute/BLAKE3/10-8                771968              1348 ns/op
BenchmarkDigestCompute/SHA256/100-8              2069121               598.8 ns/op
BenchmarkDigestCompute/BLAKE3/100-8               801408              1531 ns/op
BenchmarkDigestCompute/SHA256/1000-8              486255              2278 ns/op
BenchmarkDigestCompute/BLAKE3/1000-8              537957              4474 ns/op
BenchmarkDigestCompute/SHA256/10000-8              59762             19235 ns/op
BenchmarkDigestCompute/BLAKE3/10000-8             238616              7949 ns/op
BenchmarkDigestCompute/SHA256/100000-8              6315            188618 ns/op
BenchmarkDigestCompute/BLAKE3/100000-8             49388             24099 ns/op

not seeing the speed improvements for some reason 🤔 maybe my processor doesn't support the required instructions.

edit: I don't see any speedups running the test remotely on our executors, either 😕

@sluongng
Copy link
Contributor Author

sluongng commented Aug 9, 2023

@bduffany I think for some features you would need to set GOAMD64, which could be set using rules_go after bazel-contrib/rules_go#3251.

@sluongng
Copy link
Contributor Author

I ran some number

goos: linux
goarch: amd64
cpu: Intel(R) Xeon(R) CPU @ 3.10GHz

+----------------------------------------+--------+-------------------------+-------------------------+-------------------------+------------------------+
|                                        | Go     | V1 (default)            | V2                      | V3                      | V4                     |
+----------------------------------------+--------+-------------------------+-------------------------+-------------------------+------------------------+
|BenchmarkDigestCompute/SHA256/1-30      | 1.20.6 | 179136      5882 ns/op  | 124694      9934 ns/op  | 192846      5993 ns/op  | 189498      5734 ns/op |
|BenchmarkDigestCompute/BLAKE3/1-30      | 1.20.6 | 113416      9730 ns/op  |  80197     15101 ns/op  | 110010      9602 ns/op  | 111120      9801 ns/op |
|BenchmarkDigestCompute/SHA256/10-30     | 1.20.6 | 211686      5667 ns/op  | 172010      6050 ns/op  | 197144      5842 ns/op  | 203642      5938 ns/op |
|BenchmarkDigestCompute/BLAKE3/10-30     | 1.20.6 | 108561      9803 ns/op  | 100317     10686 ns/op  | 111651      9737 ns/op  |  95474     10881 ns/op |
|BenchmarkDigestCompute/SHA256/100-30    | 1.20.6 | 204547      5887 ns/op  | 202862      5866 ns/op  | 198572      6088 ns/op  | 175352      6641 ns/op |
|BenchmarkDigestCompute/BLAKE3/100-30    | 1.20.6 | 104535     10291 ns/op  | 104095     10765 ns/op  | 107217      9921 ns/op  | 116206     10578 ns/op |
|BenchmarkDigestCompute/SHA256/1000-30   | 1.20.6 | 150646      8687 ns/op  | 149869      8309 ns/op  | 149089      8132 ns/op  | 145069      8410 ns/op |
|BenchmarkDigestCompute/BLAKE3/1000-30   | 1.20.6 |  65079     17070 ns/op  |  83692     14930 ns/op  |  81699     13492 ns/op  |  79120     13649 ns/op |
|BenchmarkDigestCompute/SHA256/10000-30  | 1.20.6 |  36711     31821 ns/op  |  33878     31934 ns/op  |  40624     29399 ns/op  |  34269     29485 ns/op |
|BenchmarkDigestCompute/BLAKE3/10000-30  | 1.20.6 |  41028     28839 ns/op  |  34753     34579 ns/op  |  39136     29736 ns/op  |  38966     30680 ns/op |
|BenchmarkDigestCompute/SHA256/100000-30 | 1.20.6 |   4870    246121 ns/op  |   4024    254265 ns/op  |   4930    241585 ns/op  |   4788    241926 ns/op |
|BenchmarkDigestCompute/BLAKE3/100000-30 | 1.20.6 |   7570    157361 ns/op  |   7762    159516 ns/op  |   7950    152194 ns/op  |   7906    160134 ns/op |
+----------------------------------------+--------+-------------------------+-------------------------+-------------------------+------------------------+
|BenchmarkDigestCompute/SHA256/1-30      | 1.21.0 | 194114      6180 ns/op  | 193610      6618 ns/op  | 174316      6741 ns/op  | 179653      6438 ns/op |
|BenchmarkDigestCompute/BLAKE3/1-30      | 1.21.0 | 109233     10516 ns/op  | 107288     10925 ns/op  | 101058     11138 ns/op  |  97424     10827 ns/op |
|BenchmarkDigestCompute/SHA256/10-30     | 1.21.0 | 199699      6447 ns/op  | 197600      6196 ns/op  | 112714      9932 ns/op  | 180054      6699 ns/op |
|BenchmarkDigestCompute/BLAKE3/10-30     | 1.21.0 | 120045     10299 ns/op  | 115908     11777 ns/op  |  90782     11209 ns/op  |  98275     11197 ns/op |
|BenchmarkDigestCompute/SHA256/100-30    | 1.21.0 | 199070      6731 ns/op  | 178394      6316 ns/op  | 185082      6663 ns/op  | 183381      6688 ns/op |
|BenchmarkDigestCompute/BLAKE3/100-30    | 1.21.0 | 118980     10133 ns/op  |  96954     10666 ns/op  | 103338     11811 ns/op  |  96330     11648 ns/op |
|BenchmarkDigestCompute/SHA256/1000-30   | 1.21.0 | 148444      8268 ns/op  | 147850      8226 ns/op  | 140574      8349 ns/op  | 136345      8872 ns/op |
|BenchmarkDigestCompute/BLAKE3/1000-30   | 1.21.0 |  86718     12829 ns/op  |  91364     14130 ns/op  |  80214     14230 ns/op  |  74690     15546 ns/op |
|BenchmarkDigestCompute/SHA256/10000-30  | 1.21.0 |  40066     29888 ns/op  |  35396     31246 ns/op  |  38924     31039 ns/op  |  40167     30380 ns/op |
|BenchmarkDigestCompute/BLAKE3/10000-30  | 1.21.0 |  40238     29330 ns/op  |  37699     31413 ns/op  |  37322     32380 ns/op  |  35989     31762 ns/op |
|BenchmarkDigestCompute/SHA256/100000-30 | 1.21.0 |   4944    253678 ns/op  |   4918    241070 ns/op  |   4952    240906 ns/op  |   4131    243320 ns/op |
|BenchmarkDigestCompute/BLAKE3/100000-30 | 1.21.0 |   7582    164121 ns/op  |   7528    170787 ns/op  |   7676    157865 ns/op  |   7582    161672 ns/op |
+----------------------------------------+--------+-------------------------+-------------------------+-------------------------+------------------------+

The setup is in 2382fa7
which I built on top of your test @bduffany. This is run on our prod executor with Podman isolation.

Overall I think there is a noticeable speed up but it's not "huge". Not quite sure how to reduce the noise.

@bduffany
Copy link
Member

I wonder if we still aren't actually using the native SHA256 instructions, i.e. maybe useSHA is somehow false, despite the BUILD file changes. Is there a way to verify?

(But anyway I think we should merge this PR - it would be awesome if we could switch to go 1.21 if only for the min/max functions / build speed improvements 😄 )

@sluongng
Copy link
Contributor Author

@bduffany see my last comment in #4547

Our cpu does not have SHA extension.

@sluongng
Copy link
Contributor Author

Let me send a separate PR to upgrade rules_go temporarily and not having to wait for the official release, then we could unblock this.

sluongng added a commit that referenced this pull request Aug 28, 2023
Upgrade rules_go to latest commit in `master` at the moment of writing.

Unblocks Go 1.21 upgrade in #4516 by including the fixes in:
- bazel-contrib/rules_go#3660
- bazel-contrib/rules_go#3666
sluongng added a commit that referenced this pull request Aug 29, 2023
Upgrade rules_go to latest commit in `master` at the moment of writing.

Unblocks Go 1.21 upgrade in #4516 by including the fixes in:
- bazel-contrib/rules_go#3660
- bazel-contrib/rules_go#3666
@sluongng sluongng force-pushed the sluongng/go-1.21.0 branch from b2b3641 to e8896a4 Compare August 31, 2023 16:21
@sluongng sluongng marked this pull request as ready for review August 31, 2023 16:21
@sluongng
Copy link
Contributor Author

Rebasing on top of latest master to trigger CI and prepare for merge

@sluongng sluongng requested a review from bduffany August 31, 2023 18:15
@sluongng
Copy link
Contributor Author

@bduffany PTAL 🙏

@sluongng sluongng merged commit ad6ad45 into master Sep 7, 2023
@sluongng sluongng deleted the sluongng/go-1.21.0 branch September 7, 2023 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants